Re: [PATCH bpf-next v2] tracing: perf_call_bpf: use struct trace_entry in struct syscall_tp_t

[Yonghong Song <yonghong.song@linux.dev>]

On 7/31/23 1:07 AM, Yauheni Kaliuta wrote:
> Hi, Yonghong!
> 
>>>>>> On Fri, 28 Jul 2023 09:44:20 -0700, Yonghong Song  wrote:
> 
>   > On 7/28/23 7:27 AM, Yauheni Kaliuta wrote:
>   >> bpf tracepoint program uses struct trace_event_raw_sys_enter as
>   >> argument where trace_entry is the first field. Use the same instead
>   >> of unsigned long long since if it's amended (for example by RT
>   >> patch) it accesses data with wrong offset.
>   >> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
>   >> ---
>   >> v2:
>   >> - remove extra BUILD_BUG_ON
>   >> - add structure alignement
>   >> ---
>   >> kernel/trace/trace_syscalls.c | 12 ++++++++----
>   >> 1 file changed, 8 insertions(+), 4 deletions(-)
>   >> diff --git a/kernel/trace/trace_syscalls.c
>   >> b/kernel/trace/trace_syscalls.c
>   >> index 942ddbdace4a..b7139f8f4ce8 100644
>   >> --- a/kernel/trace/trace_syscalls.c
>   >> +++ b/kernel/trace/trace_syscalls.c
>   >> @@ -555,12 +555,15 @@ static int perf_call_bpf_enter(struct trace_event_call *call, struct pt_regs *re
>   >> struct syscall_trace_enter *rec)
>   >> {
>   >> struct syscall_tp_t {
>   >> -		unsigned long long regs;
>   >> +		struct trace_entry ent;
>   >> unsigned long syscall_nr;
>   >> unsigned long args[SYSCALL_DEFINE_MAXARGS];
>   >> -	} param;
>   >> +	} __aligned(8) param;
>   >> int i;
>   >> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));
> 
>   > Considering we used 'unsigned long long regs' before, should
>   > the above BUILD_BUG_ON should be
>   > 	BUILD_BUG_ON(sizeof(param.ent) < sizeof(long long));
>   > ?
> 
> Since the pointer's value is assigned I agree with Alexei (in the
> thread [1]) to use void *.

Okay, let us compare to sizeof(void *) then.

> 
>   >> +
>   >> +	/* __bpf_prog_run() requires *regs as the first parameter */
> 
>   > This comment is not correct.
> 
>   > static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
>   >                                           const void *ctx,
>   >                                           bpf_dispatcher_fn dfunc)
>   > {
>   > 	...
>   > }
> 
>   > The first parameter is 'prog'.
> 
>   > Also there is no __bpf_prog_run() referenced in this function
>   > so this comment may confuse readers. So I suggest removing
>   > this comment. The same for perf_call_bpf_exit() below.
> 
> Again, in [1] we agreed that it's better to have the comment
> since it's even more confusing.
> 
> Could you help to formulate it?
> 
> "__bpf_prog_run() requires *regs as the first argument for bpf
> prog" or something?
> 
> But yes, I can remove it of course.

You could have a comment like below:
	/* bpf prog requires 'regs' to be the first member in the ctx (a.k.a. 
&param) */

> 
>   >> *(struct pt_regs **)&param = regs;
>   >> param.syscall_nr = rec->nr;
>   >> for (i = 0; i < sys_data->nb_args; i++)
>   >> @@ -657,11 +660,12 @@ static int perf_call_bpf_exit(struct trace_event_call *call, struct pt_regs *reg
>   >> struct syscall_trace_exit *rec)
>   >> {
>   >> struct syscall_tp_t {
>   >> -		unsigned long long regs;
>   >> +		struct trace_entry ent;
>   >> unsigned long syscall_nr;
>   >> unsigned long ret;
>   >> -	} param;
>   >> +	} __aligned(8) param;
>   >> +	/* __bpf_prog_run() requires *regs as the first parameter */
>   >> *(struct pt_regs **)&param = regs;
>   >> param.syscall_nr = rec->nr;
>   >> param.ret = rec->ret;
> 
> 
> [1] https://lore.kernel.org/bpf/xunyjzy64q9b.fsf@redhat.com/T/#u
>