On Tue, Jan 28, 2025 at 07:01:47PM +0100, Christophe Leroy wrote:
> Le 27/01/2025 à 19:14, Dmitry V. Levin a écrit :
> > Since the introduction of SECCOMP_RET_TRACE support, the kernel supports
> > simultaneously both the generic kernel -ERRORCODE return value ABI and
> > the powerpc sc syscall return ABI for PTRACE_EVENT_SECCOMP tracers.
> > This change is an attempt to address the code inconsistencies in syscall
> > error return handling that were introduced as a side effect of the dual
> > ABI support.
> >
> > Signed-off-by: Dmitry V. Levin <l...@strace.io>
> > ---
> > arch/powerpc/kernel/ptrace/ptrace.c | 23 ++++++++++++++++++++---
> > arch/powerpc/kernel/signal.c | 11 +++--------
> > arch/powerpc/kernel/syscall.c | 6 +++---
> > 3 files changed, 26 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/ptrace/ptrace.c
> > b/arch/powerpc/kernel/ptrace/ptrace.c
> > index 727ed4a14545..3778775bf6ba 100644
> > --- a/arch/powerpc/kernel/ptrace/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> > @@ -207,7 +207,7 @@ static int do_seccomp(struct pt_regs *regs)
> > * syscall parameter. This is different to the ptrace ABI where
> > * both r3 and orig_gpr3 contain the first syscall parameter.
> > */
> > - regs->gpr[3] = -ENOSYS;
> > + syscall_set_return_value(current, regs, -ENOSYS, 0);
> >
> > /*
> > * We use the __ version here because we have already checked
> > @@ -215,8 +215,18 @@ static int do_seccomp(struct pt_regs *regs)
> > * have already loaded -ENOSYS into r3, or seccomp has put
> > * something else in r3 (via SECCOMP_RET_ERRNO/TRACE).
> > */
> > - if (__secure_computing(NULL))
> > + if (__secure_computing(NULL)) {
> > +
> > + /*
> > + * Traditionally, both the generic kernel -ERRORCODE return
> > + * value ABI and the powerpc sc syscall return ABI is
> > + * supported. For consistency, if the former is detected,
> > + * convert it to the latter.
> > + */
> > + if (!trap_is_scv(regs) && IS_ERR_VALUE(regs->gpr[3]))
>
> Why !trap_is_scv(regs) ? Shouldn't this also work with scv allthough it
> should be a noop ?
In trap_is_scv(regs) case both the source and the target ABIs are
-ERRORCODE so there is no subject for conversion.
> > + syscall_set_return_value(current, regs, regs->gpr[3],
> > 0);
> > return -1;
> > + }
> >
> > /*
> > * The syscall was allowed by seccomp, restore the register
> > @@ -226,6 +236,13 @@ static int do_seccomp(struct pt_regs *regs)
> > * allow the syscall to proceed.
> > */
> > regs->gpr[3] = regs->orig_gpr3;
> > + if (!trap_is_scv(regs)) {
> > + /*
> > + * Clear SO bit that was set in this function earlier by
> > + * syscall_set_return_value.
> > + */
> > + regs->ccr &= ~0x10000000L;
> > + }
>
> Can't we use syscall_set_return_value() to do that ?
Of course we could do
syscall_set_return_value(current, regs, 0, regs->orig_gpr3);
but Michael has objected to this already, see
https://lore.kernel.org/all/87jzajjde1....@mpe.ellerman.id.au/
--
ldv
