Re: [PATCH bpf-next v10 1/4] bpf: Relax tracing prog recursive attach rules

[Jiri Olsa <olsajiri@gmail.com>]

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