Re: [PATCH v7 10/11] arm64: entry: Convert to generic entry

[Kevin Brodsky <kevin.brodsky@arm.com>]

On 17/11/2025 14:30, Jinjie Ruan wrote:
> [...]
>
> diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
> index 6225981fbbdb..c91938718468 100644
> --- a/arch/arm64/include/asm/syscall.h
> +++ b/arch/arm64/include/asm/syscall.h
> @@ -9,6 +9,8 @@
>  #include <linux/compat.h>
>  #include <linux/err.h>
>  
> +#include <asm/vdso.h>
> +
>  typedef long (*syscall_fn_t)(const struct pt_regs *regs);
>  
>  extern const syscall_fn_t sys_call_table[];
> @@ -114,12 +116,30 @@ static inline int syscall_get_arch(struct task_struct *task)
>  	return AUDIT_ARCH_AARCH64;
>  }
>  
> -static inline bool has_syscall_work(unsigned long flags)
> +static inline bool arch_syscall_is_vdso_sigreturn(struct pt_regs *regs)
>  {
> -	return unlikely(flags & _TIF_SYSCALL_WORK);
> -}
> +	unsigned long vdso = (unsigned long)current->mm->context.vdso;
> +	unsigned long vdso_pages, vdso_text_len;
> +	unsigned long pc = regs->pc - 4;

On AArch32 (i.e. COMPAT), instructions may be 16-bit (in T32/Thumb), so
we shouldn't blindly use PC - 4.

>  
> -int syscall_trace_enter(struct pt_regs *regs, long syscall, unsigned long flags);
> -void syscall_exit_to_user_mode_prepare(struct pt_regs *regs);
> +#ifdef CONFIG_COMPAT
> +	if (is_compat_task()) {
> +		vdso = (unsigned long)current->mm->context.sigpage;
> +		if (pc >= vdso && pc < vdso + PAGE_SIZE)

Just return that expression (instead of true/false).

I think the approach is reasonable, as we have 4 possible trampolines in
COMPAT and they all live in a dedicated page. I don't think we need to
worry about offsetting PC, because even if it points after the last
trampoline, it will still fall within the page. IOW, just use the
unmodified value of regs->pc.

> +			return true;
> +
> +		return false;
> +	}
> +#endif
> +	if (regs->syscallno != __NR_rt_sigreturn)
> +		return false;
> +
> +	vdso_pages = (vdso_end - vdso_start) >> PAGE_SHIFT;
> +	vdso_text_len = vdso_pages << PAGE_SHIFT;
> +	if (pc < vdso || pc >= vdso + vdso_text_len)
> +		return false;

Why not use the same approach as x86 and simply check that regs->pc
points after the trampoline? We already have a way to get the address of
the vDSO's sigreturn trampoline on arm64:
VDSO_SYMBOL(current->mm->context.vdso, sigtramp) (see signal.c). The
trampoline consists of two instructions that cannot be changed (pretty
much part of the ABI), so we could compare regs->pc with sigtramp + 8.

> +
> +	return true;
> +}
>  #endif	/* __ASM_SYSCALL_H */
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index f241b8601ebd..0c083be23018 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -43,6 +43,7 @@ struct thread_info {
>  	void			*scs_sp;
>  #endif
>  	u32			cpu;
> +	unsigned long		syscall_work;   /* SYSCALL_WORK_ flags */
>  };
>  
>  #define thread_saved_pc(tsk)	\
> @@ -65,11 +66,6 @@ void arch_setup_new_exec(void);
>  #define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */
>  #define TIF_MTE_ASYNC_FAULT	6	/* MTE Asynchronous Tag Check Fault */
>  #define TIF_NOTIFY_SIGNAL	7	/* signal notifications exist */
> -#define TIF_SYSCALL_TRACE	8	/* syscall trace active */
> -#define TIF_SYSCALL_AUDIT	9	/* syscall auditing */
> -#define TIF_SYSCALL_TRACEPOINT	10	/* syscall tracepoint for ftrace */
> -#define TIF_SECCOMP		11	/* syscall secure computing */
> -#define TIF_SYSCALL_EMU		12	/* syscall emulation active */
>  #define TIF_PATCH_PENDING	13	/* pending live patching update */
>  #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
>  #define TIF_FREEZE		19
> @@ -92,30 +88,14 @@ void arch_setup_new_exec(void);
>  #define _TIF_NEED_RESCHED_LAZY	(1 << TIF_NEED_RESCHED_LAZY)
>  #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
>  #define _TIF_FOREIGN_FPSTATE	(1 << TIF_FOREIGN_FPSTATE)
> -#define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
> -#define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
> -#define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
> -#define _TIF_SECCOMP		(1 << TIF_SECCOMP)
> -#define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
>  #define _TIF_PATCH_PENDING	(1 << TIF_PATCH_PENDING)
>  #define _TIF_UPROBE		(1 << TIF_UPROBE)
> -#define _TIF_SINGLESTEP		(1 << TIF_SINGLESTEP)
>  #define _TIF_32BIT		(1 << TIF_32BIT)
>  #define _TIF_SVE		(1 << TIF_SVE)
>  #define _TIF_MTE_ASYNC_FAULT	(1 << TIF_MTE_ASYNC_FAULT)
>  #define _TIF_NOTIFY_SIGNAL	(1 << TIF_NOTIFY_SIGNAL)
>  #define _TIF_TSC_SIGSEGV	(1 << TIF_TSC_SIGSEGV)
>  
> -#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_NEED_RESCHED_LAZY | \
> -				 _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
> -				 _TIF_UPROBE | _TIF_MTE_ASYNC_FAULT | \
> -				 _TIF_NOTIFY_SIGNAL | _TIF_SIGPENDING | \
> -				 _TIF_PATCH_PENDING)

AFAICT this was already unused before this series, since commit
b3cf07851b6c ("arm64: entry: Switch to generic IRQ entry"). It should be
removed in a separate commit.

> [...]
>
> -void syscall_exit_to_user_mode_prepare(struct pt_regs *regs)
> -{
> -	unsigned long flags = read_thread_flags();
> -
> -	rseq_syscall(regs);
> -
> -	if (has_syscall_work(flags) || flags & _TIF_SINGLESTEP)

I believe switching to the generic function introduces a change here:
syscall_exit_work() is only called if a flag in SYSCALL_WORK_EXIT is
set, and this set does not include SYSCALL_EMU and SECCOMP. Practically
this means that audit_syscall_exit() will no longer be called if only
SECCOMP and/or SYSCALL_EMU is set.

It doesn't feel like a major behaviour change, but it should be pointed out.

- Kevin

> -		syscall_trace_exit(regs, flags);
> -}
>
> [...]