On Thu, Dec 21, 2023 at 09:24:37PM +0100, Dmitry Dolgov wrote:
> > On Thu, Dec 21, 2023 at 07:02:02PM +0100, Jiri Olsa wrote:
> > > + /*
> > > + * Bookkeeping for managing the program attachment chain.
> > > + *
> > > + * It might be tempting to set attach_tracing_prog flag at the attachment
> > > + * time, but this will not prevent from loading bunch of tracing prog
> > > + * first, then attach them one to another.
> >
> > hi,
> > sorry for delayed response.. this part gets trickier with every change :-)
>
> Yeah, I'm impressed how many scenarios this one-liner can affect.
>
> > > + *
> > > + * The flag attach_tracing_prog is set for the whole program lifecycle, and
> > > + * doesn't have to be cleared in bpf_tracing_link_release, since tracing
> > > + * programs cannot change attachment target.
> >
> > I'm not sure that's the case.. AFAICS the bpf_tracing_prog_attach can
> > be called on already loaded program with different target program it
> > was loaded for, like:
> >
> > load fentry1 -> bpf_test_fentry1
> >
> > load fentry2 -> fentry1
> > fentry2->attach_tracing_prog = true
> >
> > load ext1 -> prog
> >
> > attach fentry2 -> ext1
> >
> > in which case we drop the tgt_prog from loading time
> > and attach fentry2 to ext1
> >
> > but I think we could just fix with resseting the attach_tracing_prog
> > in bpf_tracing_prog_attach when the tgt_prog switch happens
> >
> > it'd be great to have test for that.. also to find out it's real case,
> > I'm not sure I haven't overlooked anything
>
> Before preparing this patch version I was confident it's possible, but
> turned out bpf_tracing_prog_attach has this condition:
>
> if (tgt_prog_fd) {
> /* For now we only allow new targets for BPF_PROG_TYPE_EXT */
> if (prog->type != BPF_PROG_TYPE_EXT) {
> err = -EINVAL;
> goto out_put_prog;
> }
>
> Here is where all such cases I've tried are failing. Just tried what
> you've described with an ext prog (reattaching fentry2 via
> bpf_link_create with target_fd and link opts containing btf_id) -- the
> same result, as well as with trying to change the fentry2 to some
> fentry3. Does it make sense to you, or do I miss anything?
ok, I was wondering what I missed ;-) looks good
>
> As as side note, I find it's generally a good idea to reset
> attach_tracing_prog in bpf_tracing_prog_attach when the tgt_prog switch
> happens. It has to do both setting it on and off, if the new target is a
> tracing/not tracing prog. The flag still will be kept during the whole
> lifetime, unless switched in bpf_tracing_prog_attach -- meaning no
> changes in bpf_tracing_link_release. If changing the attachment target
> would be possible, that would be the way to go.
agreed, you can add my ack to the next version with test fix
thanks,
jirka
