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@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));
> ?
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.
¶m) */
>> *(struct pt_regs **)¶m = 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 **)¶m = regs;
>> param.syscall_nr = rec->nr;
>> param.ret = rec->ret;
[1] https://lore.kernel.org/bpf/xunyjzy64q9b.fsf@xxxxxxxxxx/T/#u
