Re: [PATCH 2/2] powerpc: fix inconsistencies in syscall error return handling

["Dmitry V. Levin" <ldv@strace.io>]

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 <ldv@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.fsf@mpe.ellerman.id.au/


-- 
ldv