On 2025/11/19 1:14, Kevin Brodsky wrote:
> 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.
Will update it.
>
>> +
>> + 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.
Yes, it was already unused before this series
>
>> [...]
>>
>> -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.
We can add "ARCH_SYSCALL_WORK_EXIT" to be defined as "SECCOMP
||SYSCALL_EMU" to keep the behaviour unchanged.
>
> - Kevin
>
>> - syscall_trace_exit(regs, flags);
>> -}
>>
>> [...]
>
