Re: [RFC PATCH bpf-next v2] bpf: Relax tracing prog recursive attach rules

[Yonghong Song <yonghong.song@linux.dev>]

On 11/24/23 4:16 PM, Dmitry Dolgov wrote:
>> On Thu, Nov 23, 2023 at 11:24:34PM -0800, Song Liu wrote:
>>> Following the corresponding discussion [1], the reason for that is to
>>> avoid tracing progs call cycles without introducing more complex
>>> solutions. Relax "no same type" requirement to "no progs that are
>>> already an attach target themselves" for the tracing type. In this way
>>> only a standalone tracing program (without any other progs attached to
>>> it) could be attached to another one, and no cycle could be formed. To
>> 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.

IIUC, the 'prog B attached to prog A, and prog C attached to prog B'
not really possible.
    After prog B attached to prog A, we have
      prog B follower_cnt = 1
      prog A attach_depth = 1
    Then prog C wants to attach to prog B,
      since we have prog B follower_cnt = 1, then attaching will fail.

If we do have A <- B <- C chain by
    first prog C attached to prog B, and then prog B attached to A
    now we have
     prog B/C follower_cnt = 1
     prog A/B attach_depth = 1
after detaching B from A, we have
     prog B follower_cnt = 0
     prog A attach_depth = 0

In this particular case, prog B attaching to prog A should succeed
since prog B follower_cnt = 0.

Did I miss anything?

In the commit message, 'falcosecurity libs project' is mentioned as a use
case for chained fentry/fexit bpf programs. I think you should expand the
use case in more details. It is possible with use case description, people
might find better/alternative solutions for your use case.

Also, if you can have a test case to exercise your commit logic,
it will be even better.

>
>>> +       if (tgt_prog) {
>>> +               /* Bookkeeping for managing the prog attachment chain. */
>>> +               tgt_prog->aux->follower_cnt++;
>>> +               prog->aux->attach_depth = tgt_prog->aux->attach_depth + 1;
>>> +       }
>>> +
>> attach_depth is calculated at attach time, so...
>>
>>>                  struct bpf_prog_aux *aux = tgt_prog->aux;
>>>
>>> +               if (aux->attach_depth >= 32) {
>>> +                       bpf_log(log, "Target program attach depth is %d. Too large\n",
>>> +                                       aux->attach_depth);
>>> +                       return -EINVAL;
>>> +               }
>>> +
>> (continue from above) attach_depth is always 0 at program load time, no?
> Right, it's going to be always 0 for the just loaded program -- but here
> in verifier we check attach_depth of the target program, which is
> calculated at some point before. Or were you asking about something else?