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 <l...@strace.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
