linux-kernel - Re: [PATCH v7 08/11] entry: Add arch_ptrace_report_syscall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8489ee0d-8b9c-080a-04a0-b299549e86cf@huawei.com>
Date: Mon, 24 Nov 2025 17:34:47 +0800
From: Jinjie Ruan <ruanjinjie@...wei.com>
To: Kevin Brodsky <kevin.brodsky@....com>, <catalin.marinas@....com>,
	<will@...nel.org>, <oleg@...hat.com>, <tglx@...utronix.de>,
	<peterz@...radead.org>, <luto@...nel.org>, <shuah@...nel.org>,
	<kees@...nel.org>, <wad@...omium.org>, <akpm@...ux-foundation.org>,
	<ldv@...ace.io>, <macro@...am.me.uk>, <deller@....de>,
	<mark.rutland@....com>, <song@...nel.org>, <mbenes@...e.cz>,
	<ryan.roberts@....com>, <ada.coupriediaz@....com>,
	<anshuman.khandual@....com>, <broonie@...nel.org>, <pengcan@...inos.cn>,
	<dvyukov@...gle.com>, <kmal@...k.li>, <lihongbo22@...wei.com>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v7 08/11] entry: Add
 arch_ptrace_report_syscall_entry/exit()



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@....com>
>> Suggested-by: Kevin Brodsky <kevin.brodsky@....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@...utronix.de>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@...wei.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);
>>  }
>