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

From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Song Liu <song@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, yonghong.song@linux.dev,
	dan.carpenter@linaro.org, olsajiri@gmail.com
Subject: Re: [PATCH bpf-next v4 1/3] bpf: Relax tracing prog recursive attach rules
Date: Thu, 30 Nov 2023 11:08:51 +0100	[thread overview]
Message-ID: <20231130100851.fymwxhwevd3t5d7m@ddolgov.remote.csb> (raw)
In-Reply-To: <CAPhsuW6J+ZN7KQdxm+2=ZcGGkWohcQxeNS+nNjE5r0K-jdq=FQ@mail.gmail.com>

> On Wed, Nov 29, 2023 at 03:58:02PM -0800, Song Liu wrote:
> We discussed this in earlier version:
>
> "
> > If prog B attached to prog A, and prog C attached to prog B, then we
> > detach B. At this point, can we re-attach B to A?
>
> Nope, with the proposed changes it still wouldn't be possible to
> reattach B to A (if we're talking about tracing progs of course),
> because this time B is an attachment target on its own.
> "
>
> I think this can be problematic for some users. Basically, doing
> profiling on prog B can cause it to not work (cannot re-attach).

Sorry, I was probably not clear enough about this first time. Let me
elaborate:

* The patch affects only tracing programs (only they can reach the
  corresponding verifier change), so I assume in your example at least B
  and A are fentry/fexit.

* The patch is less restrictive than the current kernel implementation.
  Currently, no attach of a tracing program to another tracing program is
  possible, thus IIUC the case you describe (A, B: tracing, C -> B -> A,
  then re-attach B -> A) is not possible without the patch (the first B
  -> A is going to return a verifier error).

* I've also tried to reproduce this use case with the patch, and noticed
  that link_detach is not supported for tracing progs. Which means the
  re-attach part in (C -> B -> A) has to be done via unloading of prog B
  and C, then reattaching them one-by-one back. This is another
  limitation why the case above doesn't seem to be possible (attaching
  one-by-one back would of course work without any issues even with the
  patch).

Does it all make sense to you, or am I missing something about the
problem you describe?

> Given it is not possible to create a call circle, shall we remove
> this issue?

I was originally thinking about this when preparing the patch, even
independently of the question above, simply remove verifier limitation
for an impossible situation sounds interesting. I see the following
pros/cons:

* Just remove the verifier limitation on recursive attachment is of
  course easier.

* At the same time it makes the implementation less "defensive" against
  future changes.

* Tracking attachment depth & followers might be useful in some other
  contexts.

All in all I've decided that more elaborated approach is slightly
better. But if everyone in the community agrees that less
"defensiveness" is not an issue and verifier could be simply made less
restrictive, I'm fine with that. What do you think?