On Tue, Mar 19, 2024 at 09:09:46AM -0700, Andrii Nakryiko wrote:
> On Tue, Mar 19, 2024 at 5:48 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> >
> > On Tue, Mar 19, 2024 at 11:19:08AM +0200, Eduard Zingerman wrote:
> > > On Mon, 2024-03-18 at 11:40 -0700, Andrii Nakryiko wrote:
> > >
> > > [...]
> > >
> > > > @@ -2373,15 +2378,23 @@ static __always_inline
> > > > void __bpf_trace_run(struct bpf_raw_tp_link *link, u64 *args)
> > > > {
> > > > struct bpf_prog *prog = link->link.prog;
> > > > + struct bpf_run_ctx *old_run_ctx;
> > > > + struct bpf_trace_run_ctx run_ctx;
> > >
> > > The struct bpf_trace_run_ctx has two fields: bpf_cookie, is_uprobe
> > > (there is also run_ctx but it's size is zero).
> > > The is_uprobe field is not set by the code below.
> > > Is it necessary to zero-init `run_ctx` variable?
>
> no, not really, we need to initialize fields that are going to be
> used, and is_uprobe *shouldn't be used*, but see below
>
> > >
> >
> > I think it's ok because the is_uprobe is used by kprobes/uprobes helpers
> > while here it's used for tp programs, so it won't be used
> >
>
> yep, that's my understanding and why I didn't add is_uprobe = false,
> no need to pay for that (however the cost would be trivial, so if
> necessary it's not a problem to add it)
>
> > OTOH it might be cleaner to add special run ctx struct for tp programs,
> > (like bpf_tp_run_ctx) to avoid confusion.. but then we'd need new helper
> > version for bpf_get_attach_cookie.. perhaps just adding some explaining
> > comment will be fine
>
> so when I was implementing this, I didn't want to touch yet more code,
> but I felt it wrong that bpf_trace_run_ctx is shared between
> kprobe/uprobe and other BPF_PROG_TYPE_TRACE programs (fentry/fexit,
> tp_btf) and also perf_event/tracepoint programs. I'd say kprobe/uprobe
> should get its own, given they have these extra things like uprobe vs
> kprobe flag.
>
> But I'd like to leave it to some future clean ups, and minimize the
> amount of changes in this patch set, as I intend to backport it to a
> rather old kernel we need this functionality in.
ok, sounds good
jirka
