Re: [PATCH v7 08/11] entry: Add arch_ptrace_report_syscall_entry/exit()

[Jinjie Ruan <ruanjinjie@huawei.com>]

On 2025/11/19 1:13, Kevin Brodsky wrote:
> On 17/11/2025 14:30, Jinjie Ruan wrote:
>> Differ from generic entry, due to historical reasons, ARM64 need to
>> save/restore during syscall entry/exit because ARM64 use a scratch
>> register (ip(r12) on AArch32, x7 on AArch64) to denote syscall entry/exit.
>>
>> In preparation for moving arm64 over to the generic entry code,
>> add arch_ptrace_report_syscall_entry/exit() as the default
>> ptrace_report_syscall_entry/exit() implementation. This allows
>> arm64 to implement the architecture specific version.
>>
>> Suggested-by: Mark Rutland <mark.rutland@arm.com>
>> Suggested-by: Kevin Brodsky <kevin.brodsky@arm.com>
> 
> I don't think I suggested this patch. I see that I suggested renaming
> some functions on v3, but I don't think that justifies a Suggested-by tag.
> 
>> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  kernel/entry/syscall-common.c | 43 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/entry/syscall-common.c b/kernel/entry/syscall-common.c
>> index 66e6ba7fa80c..27310e611567 100644
>> --- a/kernel/entry/syscall-common.c
>> +++ b/kernel/entry/syscall-common.c
>> @@ -17,6 +17,25 @@ static inline void syscall_enter_audit(struct pt_regs *regs, long syscall)
>>  	}
>>  }
>>  
>> +/**
>> + * arch_ptrace_report_syscall_entry - Architecture specific
>> + *				      ptrace_report_syscall_entry().
>> + *
>> + * Invoked from syscall_trace_enter() to wrap ptrace_report_syscall_entry().
>> + * Defaults to ptrace_report_syscall_entry.
>> + *
>> + * The main purpose is to support arch-specific ptrace_report_syscall_entry()
>> + * implementation.
>> + */
>> +static __always_inline int arch_ptrace_report_syscall_entry(struct pt_regs *regs);
>> +
>> +#ifndef arch_ptrace_report_syscall_entry
>> +static __always_inline int arch_ptrace_report_syscall_entry(struct pt_regs *regs)
>> +{
>> +	return ptrace_report_syscall_entry(regs);
> 
> I saw that Thomas suggested this approach on v4, and it makes sense to
> me, but I find the naming surprising. If an architecture does need extra
> handling, then the generic function should never be called from generic
> code. So it seems to me that the more logical change would be:
> 
> * Rename: ptrace_report_syscall_entry -> __ptrace_report_syscall_entry
> * Introduce ptrace_report_syscall_entry(), defaults to
> __ptrace_report_syscall_entry()

If ptrace_report_syscall_entry() is defined in linux/ptrace.h, and an
architecture also needs to redefine this function, but the
architecture's own <asm/entry-common.h> must include <linux/ptrace.h>,
the function will end up being defined twice and cause a "duplicate
definition" compile error.

> 
> All this would be done in <linux/ptrace.h>, where it clearly belongs.
> The __ prefix makes it clear that the generic function is not the main
> interface. Even better, no need to change any caller with that approach.
> 
> - Kevin
> 
>> +}
>> +#endif
>> +
>>  long syscall_trace_enter(struct pt_regs *regs, long syscall,
>>  				unsigned long work)
>>  {
>> @@ -34,7 +53,7 @@ long syscall_trace_enter(struct pt_regs *regs, long syscall,
>>  
>>  	/* Handle ptrace */
>>  	if (work & (SYSCALL_WORK_SYSCALL_TRACE | SYSCALL_WORK_SYSCALL_EMU)) {
>> -		ret = ptrace_report_syscall_entry(regs);
>> +		ret = arch_ptrace_report_syscall_entry(regs);
>>  		if (ret || (work & SYSCALL_WORK_SYSCALL_EMU))
>>  			return -1L;
>>  	}
>> @@ -84,6 +103,26 @@ static inline bool report_single_step(unsigned long work)
>>  	return work & SYSCALL_WORK_SYSCALL_EXIT_TRAP;
>>  }
>>  
>> +/**
>> + * arch_ptrace_report_syscall_exit - Architecture specific
>> + *				     ptrace_report_syscall_exit.
>> + *
>> + * Invoked from syscall_exit_work() to wrap ptrace_report_syscall_exit().
>> + *
>> + * The main purpose is to support arch-specific ptrace_report_syscall_exit
>> + * implementation.
>> + */
>> +static __always_inline void arch_ptrace_report_syscall_exit(struct pt_regs *regs,
>> +							    int step);
>> +
>> +#ifndef arch_ptrace_report_syscall_exit
>> +static __always_inline void arch_ptrace_report_syscall_exit(struct pt_regs *regs,
>> +							    int step)
>> +{
>> +	ptrace_report_syscall_exit(regs, step);
>> +}
>> +#endif
>> +
>>  void syscall_exit_work(struct pt_regs *regs, unsigned long work)
>>  {
>>  	bool step;
>> @@ -108,5 +147,5 @@ void syscall_exit_work(struct pt_regs *regs, unsigned long work)
>>  
>>  	step = report_single_step(work);
>>  	if (step || work & SYSCALL_WORK_SYSCALL_TRACE)
>> -		ptrace_report_syscall_exit(regs, step);
>> +		arch_ptrace_report_syscall_exit(regs, step);
>>  }
>