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

[Yauheni Kaliuta <ykaliuta@redhat.com>]

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 *.

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

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

-- 
WBR,
Yauheni Kaliuta