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

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

On 25/11/2025 03:43, Jinjie Ruan wrote:
>
> On 2025/11/24 23:23, Kevin Brodsky wrote:
>> On 24/11/2025 10:34, Jinjie Ruan wrote:
>>> On 2025/11/19 1:13, Kevin Brodsky wrote:
>>>> On 17/11/2025 14:30, Jinjie Ruan wrote:
>>>>> [...]
>>>>>
>>>>> 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.
>> There's plenty of arch-defined functions in <linux/ptrace.h> already.
>> __ptrace_report_syscall_entry() should be defined inside an #ifndef and
>> architectures can define their own implementation in <asm/ptrace.h>,
>> like force_successful_syscall_return() for instance.
> Shared functions like ptrace_report_syscall() are all defined in
> <linux/ptrace.h>.
> When we want to override __ptrace_report_syscall_entry() in
> <asm/ptrace.h> we still have to include <linux/ptrace.h> again,then the
> redefine problem occurs again.
>
> What we actually need to reuse is ptrace_report_syscall_entry() (or
> __ptrace_report_syscall_entry()).

You're right, this is yet another of those circular definition problems...

> The arch version need to reuse and wrap ptrace_report_syscall_entry(),
> because for instance arm64 needs to perform additional operations before
> and after this step. Therefore, I believe the current implementation is
> appropriate.

I'm still not fond of arch_X() wrapping X() as this is unusual, but I
don't have a better idea so let's stick to that. It also makes sense to
have this done in syscall-common.c rather than a header considering the
risk of circular dependency.

- Kevin