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

[Yauheni Kaliuta <ykaliuta@redhat.com>]

Hi, Yonghong!

>>>>> On Thu, 27 Jul 2023 10:37:10 -0700, Yonghong Song  wrote:

 > On 7/27/23 8:06 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.

 > Is this 'amended by RT patch' a real thing?

Yes for me.

>> Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
 >> ---
 >> kernel/trace/trace_syscalls.c | 10 ++++++++--
 >> 1 file changed, 8 insertions(+), 2 deletions(-)
 >> diff --git a/kernel/trace/trace_syscalls.c
 >> b/kernel/trace/trace_syscalls.c
 >> index 942ddbdace4a..07f4fa395e99 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;

 > I suspect we may have issues for 32bit kernel.
 > In 32bit kernel, with the change, the alignment for
 > param could be 4. That means, the 'ctx' pointer
 > may have an alignment 4 for bpf program, if user
 > tries to do ctx->regs, which will be a mis-aligned
 > access and it may not work for all architectures.

well, will __aligned(8) save the world?

 >> int i;
 >> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));
 >> +
 >> +	/* __bpf_prog_run() requires *regs as the first parameter */
 >> *(struct pt_regs **)&param = regs;
 >> param.syscall_nr = rec->nr;
 >> for (i = 0; i < sys_data->nb_args; i++)
 >> @@ -657,11 +660,14 @@ 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;
 >> +	BUILD_BUG_ON(sizeof(param.ent) < sizeof(void *));

 > You already have BUILD_BUG_ON in perf_call_enter. There is no need
 > to have another one here.

Oh yes, thanks  :)

 >> +
 >> +	/* __bpf_prog_run() requires *regs as the first parameter */
 >> *(struct pt_regs **)&param = regs;
 >> param.syscall_nr = rec->nr;
 >> param.ret = rec->ret;


-- 
WBR,
Yauheni Kaliuta