[PATCH 2/2] powerpc: fix inconsistencies in syscall error return handling
Christophe Leroy
christophe.leroy at csgroup.eu
Tue Jan 28 18:01:47 UTC 2025
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 at 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 ?
> + 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 ?
>
> return 0;
> }
> @@ -315,7 +332,7 @@ long do_syscall_trace_enter(struct pt_regs *regs)
> * If we are aborting explicitly, or if the syscall number is
> * now invalid, set the return value to -ENOSYS.
> */
> - regs->gpr[3] = -ENOSYS;
> + syscall_set_return_value(current, regs, -ENOSYS, 0);
> return -1;
> }
>
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index aa17e62f3754..1a38d6bcaed6 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -19,6 +19,7 @@
> #include <asm/unistd.h>
> #include <asm/debug.h>
> #include <asm/tm.h>
> +#include <asm/syscall.h>
>
> #include "signal.h"
>
> @@ -229,14 +230,8 @@ static void check_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka,
> regs_add_return_ip(regs, -4);
> regs->result = 0;
> } else {
> - if (trap_is_scv(regs)) {
> - regs->result = -EINTR;
> - regs->gpr[3] = -EINTR;
> - } else {
> - regs->result = -EINTR;
> - regs->gpr[3] = EINTR;
> - regs->ccr |= 0x10000000;
> - }
> + regs->result = -EINTR;
> + syscall_set_return_value(current, regs, -EINTR, 0);
> }
> }
>
> diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
> index be159ad4b77b..2fe47191e509 100644
> --- a/arch/powerpc/kernel/syscall.c
> +++ b/arch/powerpc/kernel/syscall.c
> @@ -122,7 +122,7 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
> if (unlikely(trap_is_unsupported_scv(regs))) {
> /* Unsupported scv vector */
> _exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
> - return regs->gpr[3];
> + return regs_return_value(regs);
> }
> /*
> * We use the return value of do_syscall_trace_enter() as the
> @@ -133,13 +133,13 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
> */
> r0 = do_syscall_trace_enter(regs);
> if (unlikely(r0 >= NR_syscalls))
> - return regs->gpr[3];
> + return regs_return_value(regs);
>
> } else if (unlikely(r0 >= NR_syscalls)) {
> if (unlikely(trap_is_unsupported_scv(regs))) {
> /* Unsupported scv vector */
> _exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
> - return regs->gpr[3];
> + return regs_return_value(regs);
> }
> return -ENOSYS;
> }
More information about the Strace-devel
mailing list