| Message ID | 20220320160009.2665152-4-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user/nios2: Fix clone and sigreturn | expand |
On Sun, 20 Mar 2022 at 16:06, Richard Henderson <richard.henderson@linaro.org> wrote: > > Follow syscall_set_return_value rather than the kernel assembly > in setting the syscall return values. Only negate ret on error. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > linux-user/nios2/cpu_loop.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c > index ac71f4ee47..2ae94f4a95 100644 > --- a/linux-user/nios2/cpu_loop.c > +++ b/linux-user/nios2/cpu_loop.c > @@ -48,9 +48,18 @@ void cpu_loop(CPUNios2State *env) > env->regs[7], env->regs[8], env->regs[9], > 0, 0); > > - env->regs[2] = abs(ret); > - /* Return value is 0..4096 */ > - env->regs[7] = ret > 0xfffff000u; > + /* > + * See syscall_set_return_value. > + * Use the QEMU traditional -515 error indication in > + * preference to the < 0 indication used in entry.S. > + */ Well, it is traditional, in that we've used it for sparc for instance right back to commit 060366c5ad18b3e in 2004, and even earlier for ppc since commit 678673089d1b. probably for about as long for ppc. But *why* do we use this? Well, 516 is ERESTART_RESTARTBLOCK, and that's what the arch/sparc/kernel/entry.S code is comparing against (it does a greater-than-or-equal check, I think, hence 516, not 515). For powerpc, however, the kernel handles setting the CCR bit in syscall_exit_prepare(), and there it checks against -MAX_ERRNO. nios2, as you note in the commit message, does a < 0 check. So I think: * 515 is correct for SPARC, but we really ought not to use a hardcoded constant there * we are checking against the wrong value for PPC and should be checking MAX_ERRNO * this patch does the wrong thing for nios2 If we do the same < 0 check that the kernel does, does anything break ? thanks -- PMM
On Fri, 25 Mar 2022 at 12:12, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sun, 20 Mar 2022 at 16:06, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > Follow syscall_set_return_value rather than the kernel assembly > > in setting the syscall return values. Only negate ret on error. > > > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > --- > > linux-user/nios2/cpu_loop.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c > > index ac71f4ee47..2ae94f4a95 100644 > > --- a/linux-user/nios2/cpu_loop.c > > +++ b/linux-user/nios2/cpu_loop.c > > @@ -48,9 +48,18 @@ void cpu_loop(CPUNios2State *env) > > env->regs[7], env->regs[8], env->regs[9], > > 0, 0); > > > > - env->regs[2] = abs(ret); > > - /* Return value is 0..4096 */ > > - env->regs[7] = ret > 0xfffff000u; > > + /* > > + * See syscall_set_return_value. > > + * Use the QEMU traditional -515 error indication in > > + * preference to the < 0 indication used in entry.S. > > + */ > > Well, it is traditional, in that we've used it for sparc for > instance right back to commit 060366c5ad18b3e in 2004, and > even earlier for ppc since commit 678673089d1b. > probably for about as long for ppc. But *why* do we use this? > Well, 516 is ERESTART_RESTARTBLOCK, and that's what the > arch/sparc/kernel/entry.S code is comparing against (it does a > greater-than-or-equal check, I think, hence 516, not 515). > > For powerpc, however, the kernel handles setting the CCR > bit in syscall_exit_prepare(), and there it checks against > -MAX_ERRNO. This turns out to be because in 2015 kernel commit c3525940cca5 switched powerpc from checking against 515/516 and instead made them check MAX_ERRNO (4095). (If anybody cared about seccomp on sparc hosts they'd probably want to fix the sparc kernel similarly, but presumably nobody does :-)) The kernel commit message mentions some infrastructure in the form of force_successful_syscall_return() where syscall implementations can force that a value above -MAX_ERRNO is still treated as "success". In theory perhaps we should have something similar... -- PMM
On 3/25/22 07:37, Peter Maydell wrote: > This turns out to be because in 2015 kernel commit c3525940cca5 > switched powerpc from checking against 515/516 and instead made > them check MAX_ERRNO (4095). > > (If anybody cared about seccomp on sparc hosts they'd probably > want to fix the sparc kernel similarly, but presumably nobody > does :-)) Indeed, thanks for the archaeology. > The kernel commit message mentions some infrastructure in > the form of force_successful_syscall_return() where syscall > implementations can force that a value above -MAX_ERRNO > is still treated as "success". In theory perhaps we should > have something similar... In theory, yes. It affects 3 or 4 syscalls. That said, nios2 doesn't define force_successful_syscall_return. :-P r~
diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c index ac71f4ee47..2ae94f4a95 100644 --- a/linux-user/nios2/cpu_loop.c +++ b/linux-user/nios2/cpu_loop.c @@ -48,9 +48,18 @@ void cpu_loop(CPUNios2State *env) env->regs[7], env->regs[8], env->regs[9], 0, 0); - env->regs[2] = abs(ret); - /* Return value is 0..4096 */ - env->regs[7] = ret > 0xfffff000u; + /* + * See syscall_set_return_value. + * Use the QEMU traditional -515 error indication in + * preference to the < 0 indication used in entry.S. + */ + if (ret > (abi_ulong)-515) { + env->regs[2] = -ret; + env->regs[7] = 1; + } else { + env->regs[2] = ret; + env->regs[7] = 0; + } break; case 1:
Follow syscall_set_return_value rather than the kernel assembly in setting the syscall return values. Only negate ret on error. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/nios2/cpu_loop.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)