On Sun, 2026-07-12 at 23:25 +0200, Thomas Gleixner wrote: > The return values of syscall_enter_from_user_mode[_work]() are > non-intuitive. Both functions return the syscall number which should be > invoked by the architecture specific syscall entry code. The returned > number can be: > > - the unmodified syscall number which was handed in by the caller > > - a modified syscall number (ptrace, seccomp, trace/probe/bpf) > > That has an additional twist. If the return value is -1L then the caller is > not allowed to modify the return value as that indicates that the modifying > entity requests to abort the syscall and set the return value already. That > can obviously not be differentiated from a syscall which handed in -1 as > syscall number. > > The most trivial way to deal with that is: > > set_return_value(regs, -ENOSYS); > nr = syscall_enter_from_user_mode(regs, nr); > if (valid(nr)) > handle_syscall(regs, nr); > > That's what LOONGARCH, RISCV, and X86 do. But PowerPC and S390 do not > preset the return value, so when user space hands in -1 and there is > nothing setting the return value in the entry work code, then the syscall > is skipped but the return value is whatever random data has been in the > return value register. > > Change the return values of syscall_enter_from_user_mode[_work]() to > boolean and return false, when either ptrace or seccomp request to skip the > syscall. If they return true, update the syscall number as it might have > been changed. > > That results in slightly different behaviour of the architectures versus > tracing. > > If the syscall tracepoint has probe/BPF attached, those might set the > syscall number to -1 and also set the return value. PowerPC and S390 will > then overwrite that value with -ENOSYS. The other architectures will just > ignore it like any other invalid syscall and use the modified one. > > Originally-by: Michal Suchánek <[email protected]> > Signed-off-by: Thomas Gleixner <[email protected]> > --- > V2: Change the return logic so Power and S390 can insist on being special. > --- > Documentation/core-api/entry.rst | 45 > ++++++++++++++++++++++++++++++--------- > arch/loongarch/kernel/syscall.c | 14 ++++++------ > arch/powerpc/kernel/syscall.c | 3 +- > arch/riscv/kernel/traps.c | 11 ++++----- > arch/s390/kernel/syscall.c | 7 ++++-- > arch/x86/entry/syscall_32.c | 25 ++++++++++----------- > arch/x86/entry/syscall_64.c | 12 +++++----- > include/linux/entry-common.h | 32 +++++++++++++-------------- > 8 files changed, 88 insertions(+), 61 deletions(-)
I've been staring at this for a while. My only concern is that for powerpc and s390, which do not set a default return value, the return value is no longer forced to -ENOSYS for the cases when syscall_trace_enter() returns False. Previously, it was turned into a - 1 in syscall_enter_from_user_mode_work() and interpreted as a syscall number, so the architecture specific code took the invalid syscall number path where it set the return value to -ENOSYS explicitly. That implies that in all 3 cases when syscall_trace_enter() returns False, the underlying function that denied the syscall *must* also set the syscall return value (or deliver a signal or rollback the syscall), or else the return value would be junk. That *seems* to be the case as far as I can tell, but the ramifications are quite deep, and I must admit I don't fully understand everything. With that said, Reviewed-by: Radu Rendec <[email protected]> > --- a/Documentation/core-api/entry.rst > +++ b/Documentation/core-api/entry.rst > @@ -58,26 +58,51 @@ state transitions must run with interrup > Syscalls > -------- > > -Syscall-entry code starts in assembly code and calls out into low-level C > code > -after establishing low-level architecture-specific state and stack frames. > This > -low-level C code must not be instrumented. A typical syscall handling > function > -invoked from low-level assembly code looks like this: > +Syscall-entry code starts in assembly code and calls out into low-level C > +code after establishing low-level architecture-specific state and stack > +frames. This low-level C code must not be instrumented. The recommended > +syscall handling function invoked from low-level assembly code looks like > +this: > > .. code-block:: c > > - noinstr void syscall(struct pt_regs *regs, int nr) > + noinstr void syscall(struct pt_regs *regs, long nr) > { > arch_syscall_enter(regs); > - nr = syscall_enter_from_user_mode_randomize_stack(regs, nr); > + result_reg(regs) = -ENOSYS; > + if (syscall_enter_from_user_mode_randomize_stack(regs, &nr)) { > + instrumentation_begin(); > + if (valid(nr) > + result_reg(regs) = invoke_syscall(regs, nr); > + instrumentation_end(); > + } > + syscall_exit_to_user_mode(regs); > + } > > - instrumentation_begin(); > - if (!invoke_syscall(regs, nr) && nr != -1) > - result_reg(regs) = __sys_ni_syscall(regs); > - instrumentation_end(); > +This is the most resilent variant as it has always a guaranteed valid > +return code. The alternative variant is: > + > +.. code-block:: c > > + noinstr void syscall(struct pt_regs *regs, long nr) > + { > + arch_syscall_enter(regs); > + if (syscall_enter_from_user_mode_randomize_stack(regs, &nr)) { > + instrumentation_begin(); > + if (valid(nr) > + result_reg(regs) = invoke_syscall(regs, nr); > + else > + result_reg(regs) = -ENOSYS; > + instrumentation_end(); > + } > syscall_exit_to_user_mode(regs); > } > > +That works for most situations except when a probe/BPF attached to the > +syscall tracepoint sets an invalid syscall number e.g. -1 and also modifies > +the result register. So this variant will obviously overwrite the modified > +result with -ENOSYS. > + > syscall_enter_from_user_mode_randomize_stack() first invokes > enter_from_user_mode_randomize_stack() which establishes state in the > following order: > --- a/arch/loongarch/kernel/syscall.c > +++ b/arch/loongarch/kernel/syscall.c > @@ -57,8 +57,8 @@ typedef long (*sys_call_fn)(unsigned lon > > void noinstr __no_stack_protector do_syscall(struct pt_regs *regs) > { > - unsigned long nr; > sys_call_fn syscall_fn; > + unsigned long nr; > > nr = regs->regs[11]; > /* Set for syscall restarting */ > @@ -69,12 +69,12 @@ void noinstr __no_stack_protector do_sys > regs->orig_a0 = regs->regs[4]; > regs->regs[4] = -ENOSYS; > > - nr = syscall_enter_from_user_mode_randomize_stack(regs, nr); > - > - if (nr < NR_syscalls) { > - syscall_fn = sys_call_table[array_index_nospec(nr, > NR_syscalls)]; > - regs->regs[4] = syscall_fn(regs->orig_a0, regs->regs[5], > regs->regs[6], > - regs->regs[7], regs->regs[8], > regs->regs[9]); > + if (likely(syscall_enter_from_user_mode_randomize_stack(regs, &nr))) { > + if (nr < NR_syscalls) { > + syscall_fn = sys_call_table[array_index_nospec(nr, > NR_syscalls)]; > + regs->regs[4] = syscall_fn(regs->orig_a0, > regs->regs[5], regs->regs[6], > + regs->regs[7], > regs->regs[8], regs->regs[9]); > + } > } > > syscall_exit_to_user_mode(regs); > --- a/arch/powerpc/kernel/syscall.c > +++ b/arch/powerpc/kernel/syscall.c > @@ -18,7 +18,8 @@ notrace long system_call_exception(struc > long ret; > syscall_fn f; > > - r0 = syscall_enter_from_user_mode_randomize_stack(regs, r0); > + if (unlikely(!syscall_enter_from_user_mode_randomize_stack(regs, &r0))) > + return syscall_get_error(current, regs); > > if (unlikely(r0 >= NR_syscalls)) { > if (unlikely(trap_is_unsupported_scv(regs))) { > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -332,13 +332,12 @@ void do_trap_ecall_u(struct pt_regs *reg > > riscv_v_vstate_discard(regs); > > - syscall = syscall_enter_from_user_mode_randomize_stack(regs, > syscall); > - > - if (syscall >= 0 && syscall < NR_syscalls) { > - syscall = array_index_nospec(syscall, NR_syscalls); > - syscall_handler(regs, syscall); > + if (likely(syscall_enter_from_user_mode_randomize_stack(regs, > &syscall))) { > + if (syscall >= 0 && syscall < NR_syscalls) { > + syscall = array_index_nospec(syscall, > NR_syscalls); > + syscall_handler(regs, syscall); > + } > } > - > syscall_exit_to_user_mode(regs); > } else { > irqentry_state_t state = irqentry_nmi_enter(regs); > --- a/arch/s390/kernel/syscall.c > +++ b/arch/s390/kernel/syscall.c > @@ -96,6 +96,7 @@ SYSCALL_DEFINE0(ni_syscall) > void noinstr __do_syscall(struct pt_regs *regs, int per_trap) > { > unsigned long nr; > + bool permit; > > enter_from_user_mode_randomize_stack(regs); > > @@ -121,7 +122,9 @@ void noinstr __do_syscall(struct pt_regs > regs->psw.addr = current->restart_block.arch_data; > current->restart_block.arch_data = 1; > } > - nr = syscall_enter_from_user_mode_work(regs, nr); > + > + permit = syscall_enter_from_user_mode_work(regs, &nr); > + > /* > * In the s390 ptrace ABI, both the syscall number and the return value > * use gpr2. However, userspace puts the syscall number either in the > @@ -129,7 +132,7 @@ void noinstr __do_syscall(struct pt_regs > * work, the ptrace code sets PIF_SYSCALL_RET_SET, which is checked here > * and if set, the syscall will be skipped. > */ > - if (unlikely(test_and_clear_pt_regs_flag(regs, PIF_SYSCALL_RET_SET))) > + if (unlikely(test_and_clear_pt_regs_flag(regs, PIF_SYSCALL_RET_SET) || > !permit)) > goto out; > regs->gprs[2] = -ENOSYS; > if (likely(nr < NR_syscalls)) { > --- a/arch/x86/entry/syscall_32.c > +++ b/arch/x86/entry/syscall_32.c > @@ -161,8 +161,9 @@ static __always_inline bool int80_is_ext > nr = syscall_32_enter(regs); > > local_irq_enable(); > - nr = syscall_enter_from_user_mode_work(regs, nr); > - do_syscall_32_irqs_on(regs, nr); > + > + if (likely(syscall_enter_from_user_mode_work(regs, &nr))) > + do_syscall_32_irqs_on(regs, nr); > > instrumentation_end(); > syscall_exit_to_user_mode(regs); > @@ -223,8 +224,8 @@ DEFINE_FREDENTRY_RAW(int80_emulation) > nr = syscall_32_enter(regs); > > local_irq_enable(); > - nr = syscall_enter_from_user_mode_work(regs, nr); > - do_syscall_32_irqs_on(regs, nr); > + if (likely(syscall_enter_from_user_mode_work(regs, &nr))) > + do_syscall_32_irqs_on(regs, nr); > > instrumentation_end(); > syscall_exit_to_user_mode(regs); > @@ -243,13 +244,13 @@ DEFINE_FREDENTRY_RAW(int80_emulation) > * orig_ax, the int return value truncates it. This matches > * the semantics of syscall_get_nr(). > */ > - nr = syscall_enter_from_user_mode_randomize_stack(regs, nr); > - > - instrumentation_begin(); > + if (likely(syscall_enter_from_user_mode_randomize_stack(regs, &nr))) { > + instrumentation_begin(); > > - do_syscall_32_irqs_on(regs, nr); > + do_syscall_32_irqs_on(regs, nr); > > - instrumentation_end(); > + instrumentation_end(); > + } > syscall_exit_to_user_mode(regs); > } > #endif /* !CONFIG_IA32_EMULATION */ > @@ -286,10 +287,8 @@ static noinstr bool __do_fast_syscall_32 > return false; > } > > - nr = syscall_enter_from_user_mode_work(regs, nr); > - > - /* Now this is just like a normal syscall. */ > - do_syscall_32_irqs_on(regs, nr); > + if (likely(syscall_enter_from_user_mode_work(regs, &nr))) > + do_syscall_32_irqs_on(regs, nr); > > instrumentation_end(); > syscall_exit_to_user_mode(regs); > --- a/arch/x86/entry/syscall_64.c > +++ b/arch/x86/entry/syscall_64.c > @@ -78,14 +78,14 @@ static __always_inline void do_syscall_x > /* Returns true to return using SYSRET, or false to use IRET */ > __visible noinstr bool do_syscall_64(struct pt_regs *regs, long nr) > { > - nr = syscall_enter_from_user_mode_randomize_stack(regs, nr); > + if (likely(syscall_enter_from_user_mode_randomize_stack(regs, &nr))) { > + instrumentation_begin(); > > - instrumentation_begin(); > + if (!do_syscall_x64(regs, nr)) > + do_syscall_x32(regs, nr); > > - if (!do_syscall_x64(regs, nr)) > - do_syscall_x32(regs, nr); > - > - instrumentation_end(); > + instrumentation_end(); > + } > syscall_exit_to_user_mode(regs); > > /* > --- a/include/linux/entry-common.h > +++ b/include/linux/entry-common.h > @@ -111,16 +111,15 @@ static __always_inline long syscall_trac > * @regs: Pointer to currents pt_regs > * @syscall: The syscall number > * > - * Invoked from architecture specific syscall entry code with interrupts > - * enabled after invoking enter_from_user_mode(), enabling interrupts and > - * extra architecture specific work. > + * Invoked from architecture specific syscall entry code with interrupts > enabled > + * after invoking enter_from_user_mode(), enabling interrupts and extra > + * architecture specific work with the syscall return value preset to > -ENOSYS. > * > - * Returns: The original or a modified syscall number > + * Returns: True if the syscall should be invoked, False otherwise. > * > - * If the returned syscall number is -1 then the syscall should be > - * skipped. In this case the caller may invoke syscall_set_error() or > - * syscall_set_return_value() first. If neither of those are called and -1 > - * is returned, then the syscall will fail with ENOSYS. > + * If the return value is false, the caller must skip the syscall and leave > the > + * syscall return value unmodified as it might have been set by one of the > entry > + * work functions. > * > * It handles the following work items: > * > @@ -128,19 +127,20 @@ static __always_inline long syscall_trac > * ptrace_report_syscall_permit_entry(), __seccomp_permit_syscall(), > trace_sys_enter() > * 2) Invocation of audit_syscall_entry() > */ > -static __always_inline long syscall_enter_from_user_mode_work(struct pt_regs > *regs, long syscall) > +static __always_inline bool syscall_enter_from_user_mode_work(struct pt_regs > *regs, long *syscall) > { > unsigned long work = READ_ONCE(current_thread_info()->syscall_work); > > - if (work & SYSCALL_WORK_ENTER) { > - if (!syscall_trace_enter(regs, work, syscall)) > - return -1L; > + if (!(work & SYSCALL_WORK_ENTER)) > + return true; > > - /* Reread the syscall number as it might have been modified */ > - syscall = syscall_get_nr(current, regs); > - } > + if (unlikely(!syscall_trace_enter(regs, work, *syscall))) > + return false; > > - return syscall; > + /* Reread the syscall number as it might have been modified */ > + *syscall = syscall_get_nr(current, regs); > + > + return true; > } > > /**
