On Wed, Dec 7, 2022 at 12:18 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Tue, Dec 06, 2022 at 06:14:06PM -0800, Namhyung Kim wrote:
>
> SNIP
>
> > -static int __bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog)
> > +static void *bpf_trace_norecurse_funcs[12] = {
> > + (void *)bpf_trace_run_norecurse1,
> > + (void *)bpf_trace_run_norecurse2,
> > + (void *)bpf_trace_run_norecurse3,
> > + (void *)bpf_trace_run_norecurse4,
> > + (void *)bpf_trace_run_norecurse5,
> > + (void *)bpf_trace_run_norecurse6,
> > + (void *)bpf_trace_run_norecurse7,
> > + (void *)bpf_trace_run_norecurse8,
> > + (void *)bpf_trace_run_norecurse9,
> > + (void *)bpf_trace_run_norecurse10,
> > + (void *)bpf_trace_run_norecurse11,
> > + (void *)bpf_trace_run_norecurse12,
> > +};
> > +
> > +static int __bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog,
> > + void *func, void *data)
> > {
> > struct tracepoint *tp = btp->tp;
> >
> > @@ -2325,13 +2354,12 @@ static int __bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *
> > if (prog->aux->max_tp_access > btp->writable_size)
> > return -EINVAL;
> >
> > - return tracepoint_probe_register_may_exist(tp, (void *)btp->bpf_func,
> > - prog);
> > + return tracepoint_probe_register_may_exist(tp, func, data);
> > }
> >
> > int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog)
> > {
> > - return __bpf_probe_register(btp, prog);
> > + return __bpf_probe_register(btp, prog, btp->bpf_func, prog);
> > }
> >
> > int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_prog *prog)
> > @@ -2339,6 +2367,33 @@ int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_prog *prog)
> > return tracepoint_probe_unregister(btp->tp, (void *)btp->bpf_func, prog);
> > }
> >
> > +int bpf_probe_register_norecurse(struct bpf_raw_event_map *btp, struct bpf_prog *prog,
> > + struct bpf_raw_event_data *data)
> > +{
> > + void *bpf_func;
> > +
> > + data->active = alloc_percpu_gfp(int, GFP_KERNEL);
> > + if (!data->active)
> > + return -ENOMEM;
> > +
> > + data->prog = prog;
> > + bpf_func = bpf_trace_norecurse_funcs[btp->num_args];
> > + return __bpf_probe_register(btp, prog, bpf_func, data);
>
> I don't think we can do that, because it won't do the arg -> u64 conversion
> that __bpf_trace_##call functions are doing:
>
> __bpf_trace_##call(void *__data, proto) \
> { \
> struct bpf_prog *prog = __data; \
> CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(prog, CAST_TO_U64(args)); \
> }
>
> like for 'old_pid' arg in sched_process_exec tracepoint:
>
> ffffffff811959e0 <__bpf_trace_sched_process_exec>:
> ffffffff811959e0: 89 d2 mov %edx,%edx
> ffffffff811959e2: e9 a9 07 14 00 jmp ffffffff812d6190 <bpf_trace_run3>
> ffffffff811959e7: 66 0f 1f 84 00 00 00 nopw 0x0(%rax,%rax,1)
> ffffffff811959ee: 00 00
>
> bpf program could see some trash in args < u64
>
> we'd need to add 'recursion' variant for all __bpf_trace_##call functions
Ah, ok. So 'contention_begin' tracepoint has unsigned int flags.
perf lock contention BPF program properly uses the lower 4 bytes of flags,
but others might access the whole 8 bytes then they will see the garbage.
Is that your concern?
Hmm.. I think we can use BTF to get the size of each argument then do
the conversion. Let me see..
Thanks,
Namhyung
