Message-ID: <20250123221646.GA22814@strace.io>
Date: Fri, 24 Jan 2025 00:16:46 +0200
From: "Dmitry V. Levin" <ldv@...ace.io>
To: Eugene Syromyatnikov <evgsyr@...il.com>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>,
	Alexey Gladkov <legion@...nel.org>, Oleg Nesterov <oleg@...hat.com>,
	Michael Ellerman <mpe@...erman.id.au>,
	Mike Frysinger <vapier@...too.org>,
	Renzo Davoli <renzo@...unibo.it>,
	Davide Berardi <berardi.dav@...il.com>,
	strace-devel@...ts.strace.io,
	Madhavan Srinivasan <maddy@...ux.ibm.com>,
	Nicholas Piggin <npiggin@...il.com>,
	Naveen N Rao <naveen@...nel.org>, linuxppc-dev@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/7] powerpc: properly negate error in
 syscall_set_return_value()

On Thu, Jan 23, 2025 at 08:11:44PM +0100, Eugene Syromyatnikov wrote:
> On Thu, Jan 23, 2025 at 7:28 PM Dmitry V. Levin <ldv@...ace.io> wrote:
> > Indeed, there is an inconsistency in !trap_is_scv case.
> >
> > In some places such as syscall_get_error() and regs_return_value() the
> > semantics is as I described earlier: gpr[3] contains a positive ERRORCODE
> > and ccr has 0x10000000 flag set.  This semantics is a part of the ABI and
> > therefore cannot be changed.
> >
> > In some other places like do_seccomp() and do_syscall_trace_enter() the
> > semantics is similar to the trap_is_scv case: gpr[3] contains a negative
> > ERRORCODE and ccr is unchanged.  In addition, system_call_exception()
> > returns the system call function return value when it is executed, and
> > gpr[3] otherwise.  The value returned by system_call_exception() is passed
> > on to syscall_exit_prepare() which performs the conversion you mentioned.
> >
> > What's remarkable is that in those places that are a part of the ABI the
> > traditional semantics is kept, while in other places the implementation
> > follows the trap_is_scv-like semantics, while traditional semantics is
> > also supported there.
> >
> > The only case where I see some intersection is do_seccomp() where the
> > tracer would be able to see -ENOSYS in gpr[3].  However, the seccomp stop
> > is not the place where the tracer *reads* the system call exit status,
> > so whatever was written in gpr[3] before __secure_computing() is not
> > really relevant, consequently, selftests/seccomp/seccomp_bpf passes with
> > this patch applied as well as without it.
> >
> > After looking at system_call_exception() I doubt this inconsistency can be
> > easily avoided, so I don't see how this patch could be enhanced further,
> > and what else could I do with the patch besides dropping it and letting
> > !trap_is_scv case be unsupported by PTRACE_SET_SYSCALL_INFO API, which
> > would be unfortunate.
> 
> The semantics of r3 on syscall return (including the negatedness of
> the errno value) is documented in [1] (at least for the 64-bit case,
> but I conjecture the 32-bit one is the same, sans the lack of the v2
> ABI and scv there), so I would suggest to consider any deviation from
> that a kernel programming error to be fixed.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/powerpc/syscall64-abi.rst?id=v6.13#n30

The semantics of r3 on syscall return is correct, thanks to
syscall_exit_prepare() that performs necessary manipulations with gpr[3].

What's wrong on powerpc in !trap_is_scv case is that its current
implementation of syscall_set_return_value() follows a different semantics,
making it unusable on syscall return.  While syscall_set_return_value() was
used only on entering syscall via do_seccomp(), it was not a problem yet.
It became a problem when we started to use it on syscall return, in the
same state when its sibling syscall_get_error() is used.  Note that among
all the architectures in the kernel tree powerpc in !trap_is_scv case is
the only one that has this problem.  My patch is intended to address this
without breaking anything else.


-- 
ldv

Powered by blists - more mailing lists