Re: [patch 18/18] entry, treewide: Make syscall_enter_from_user_mode[_work]() indicate syscall execution

[Shrikanth Hegde <sshegde@linux.ibm.com>]

Hi Thomas.

On 7/8/26 12:37 AM, Thomas Gleixner wrote:
> From: Michal Suchánek <msuchanek@suse.de>
> 
> 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 established way to deal with that is:
> 
>      set_return_value(regs, -ENOSYS);
>      nr = syscall_enter_from_user_mode(regs, nr);
>      if ((unsigned)nr < SYSCALLNR_MAX)
>      	handle_syscall(regs, nr);
>      else if (nr != -1)
>      	set_return_value(regs, -ENOSYS);
> 
> The latter is obviously redundant, but that's just a leftover of the
> historical evolution of this code. S390 has some special requirements here,
> which can be avoided when the return value is not ambiguous.
> 
> Now that the functions which modify the syscall number and want to abort
> are converted to indicate that with a boolean return value, it's obvious to
> hand this through to the callers.
> 
> Rework syscall_enter_from_user_mode[_work]) so they take a pointer to the
> syscall number and return a boolean, which indicates whether the syscall
> should be handled or not.
> 
> That's not only more intuitive, it also results in slightly denser
> executable code on x86 at least, but perf results are neutral and within
> the noise.
> 
> [ tglx: Adopted it to the changes in the generic entry code, fixed up the
>    	32-bit fallout and rewrote change log ]
> 
> Signed-off-by: Michal Suchánek <msuchanek@suse.de>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Huacai Chen <chenhuacai@kernel.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Shrikanth Hegde <sshegde@linux.ibm.com>
> Cc: Paul Walmsley <pjw@kernel.org>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: linux-doc@vger.kernel.org
> Cc: loongarch@lists.linux.dev
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-s390@vger.kernel.org
> ---
>   Documentation/core-api/entry.rst |   18 +++++++++++-------
>   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     |   12 +++++-------
>   8 files changed, 53 insertions(+), 49 deletions(-)
> 
> --- a/Documentation/core-api/entry.rst
> +++ b/Documentation/core-api/entry.rst
> @@ -68,16 +68,20 @@ low-level C code must not be instrumente
>     noinstr void syscall(struct pt_regs *regs, int nr)
>     {
>   	arch_syscall_enter(regs);
> -	nr = syscall_enter_from_user_mode_randomize_stack(regs, nr);
> -
> -	instrumentation_begin();
> -	if (!invoke_syscall(regs, nr) && nr != -1)
> -	 	result_reg(regs) = __sys_ni_syscall(regs);
> -	instrumentation_end();
> -
> +	result_reg(regs) = -ENOSYS;
> +	if (syscall_enter_from_user_mode_randomize_stack(regs, &nr)) {
> +		instrumentation_begin();
> +		if (!invoke_syscall(regs, nr))
> +			result_reg(regs) = __sys_ni_syscall(regs);
> +		instrumentation_end();
> +	}
>   	syscall_exit_to_user_mode(regs);
>     }
>   
> +It is required that either the low level assembly code or the syscall
> +function sets the result register to -ENOSYS before invoking the generic
> +code.
> +
>   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);
>   

There is one missing )

diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
index d85894bdb6a2..1440dcabe052 100644
--- a/arch/powerpc/kernel/syscall.c
+++ b/arch/powerpc/kernel/syscall.c
@@ -18,7 +18,7 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
         long ret;
         syscall_fn f;
  
-       if (unlikely(!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(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 (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
> @@ -71,7 +71,7 @@ static inline void syscall_enter_audit(s
>   	}
>   }
>   
> -static __always_inline bool syscall_trace_enter(struct pt_regs *regs, unsigned long work,
> +static __always_inline long syscall_trace_enter(struct pt_regs *regs, unsigned long work,
>   						long *syscall)
>   {
>   	/*
> @@ -141,16 +141,14 @@ static __always_inline bool 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 (unlikely(work & SYSCALL_WORK_ENTER))
> +		return syscall_trace_enter(regs, work, syscall);
>   
> -	return syscall;
> +	return true;
>   }
>   
>   /**
>