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

[Yonghong Song <yonghong.song@xxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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));
?

> +
> +	/* __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.

>   	*(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;