On Sat, Nov 25, 2023 at 11:55 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> 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
I think prog A has follower_cnt=1, while prog B has follow_cnt=0, no?
Thanks,
Song
> 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?
