Re: [PATCH bpf-next v2] bpf: Log error code on trampoline unlink failure

[xu.xin16]

> > Replace silent WARN_ON_ONCE with WARN_ONCE that prints the actual error
> > code from bpf_trampoline_unlink_prog(). This aids debugging of race
> > conditions during link teardown, while keeping the warning rate limited
> > to avoid log flooding.
> > 
> > This will be very helpful for speeding up trouble-shooting of some crash
> > UAF due to bpf_trampoline_unlink_prog failures.
> > 
> > No functional change intended.
> 
> Is it "No change to unlink behavior"?

Yes.

> 
> > 
> > Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
> > ---
> > v1->v2:
> > 1) clean the subject name by remove 'syscall' suggested by Leon Hwang
> > https://lore.kernel.org/all/20a444b2-aeed-4af8-ba76-e994e2c14087@xxxxxxxxx/
> > 2) Add up the missed case in kernel/bpf/trampoline.c:bpf_shim_tramp_link_release()
> > 
> > kernel/bpf/syscall.c    | 9 ++++++---
> >  kernel/bpf/trampoline.c | 5 ++++-
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 6db306d23b47..2348dc33abf4 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3626,10 +3626,13 @@ static void bpf_tracing_link_release(struct bpf_link *link)
> >  {
> >      struct bpf_tracing_link *tr_link =
> >          container_of(link, struct bpf_tracing_link, link.link);
> > +    int err;
> > 
> > -    WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link.node,
> > -                        tr_link->trampoline,
> > -                        tr_link->tgt_prog));
> > +    err = bpf_trampoline_unlink_prog(&tr_link->link.node,
> > +                    tr_link->trampoline,
> > +                    tr_link->tgt_prog);
> > +    if (err)
> > +        WARN_ONCE(err, "bpf_trampoline_unlink_prog returns error: %d\n", err);
> 
> 'if (err)' here is unnecessary.
> 
> > 
> >      bpf_trampoline_put(tr_link->trampoline);
> > 
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 1a721fc4bef5..dae3c2104ed4 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -997,12 +997,15 @@ static void bpf_shim_tramp_link_release(struct bpf_link *link)
> >  {
> >      struct bpf_shim_tramp_link *shim_link =
> >          container_of(link, struct bpf_shim_tramp_link, link.link);
> > +    int err;
> > 
> >      /* paired with 'shim_link->trampoline = tr' in bpf_trampoline_link_cgroup_shim */
> >      if (!shim_link->trampoline)
> >          return;
> > 
> > -    WARN_ON_ONCE(bpf_trampoline_unlink_prog(&shim_link->link.node, shim_link->trampoline, NULL));
> > +    err = bpf_trampoline_unlink_prog(&shim_link->link.node, shim_link->trampoline, NULL);
> > +    if (err)
> > +        WARN_ONCE(err, "bpf_trampoline_unlink_prog returns error: %d\n", err);
> 
> Ditto.
> 
> >      bpf_trampoline_put(shim_link->trampoline);
> >  }
> > 
> 
> Would it be better to include this multi_detach() one?
> 
>         WARN_ON_ONCE(__bpf_trampoline_unlink_prog(&mnode->node, mnode->trampoline,
>                     NULL, &trampoline_multi_ops, data));
> 

Sure, what about moving WARNing into __bpf_trampoline_unlink_prog and then the failures of
__bpf_trampoline_unlink_prog everywhere will be logged.