Re: [PATCHv4 bpf-next 1/5] bpf: Allow trampoline re-attach for tracing and lsm programs

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Apr 13, 2021 at 03:03:27PM -0700, Andrii Nakryiko wrote:
> On Mon, Apr 12, 2021 at 9:28 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently we don't allow re-attaching of trampolines. Once
> > it's detached, it can't be re-attach even when the program
> > is still loaded.
> >
> > Adding the possibility to re-attach the loaded tracing and
> > lsm programs.
> >
> > Fixing missing unlock with proper cleanup goto jump reported
> > by Julia.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> > Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/syscall.c    | 23 +++++++++++++++++------
> >  kernel/bpf/trampoline.c |  2 +-
> >  2 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 6428634da57e..f02c6a871b4f 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2645,14 +2645,25 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> >          *   target_btf_id using the link_create API.
> >          *
> >          * - if tgt_prog == NULL when this function was called using the old
> > -         *   raw_tracepoint_open API, and we need a target from prog->aux
> > -         *
> > -         * The combination of no saved target in prog->aux, and no target
> > -         * specified on load is illegal, and we reject that here.
> > +        *   raw_tracepoint_open API, and we need a target from prog->aux
> > +        *
> > +        * - if prog->aux->dst_trampoline and tgt_prog is NULL, the program
> > +        *   was detached and is going for re-attachment.
> >          */
> >         if (!prog->aux->dst_trampoline && !tgt_prog) {
> > -               err = -ENOENT;
> > -               goto out_unlock;
> > +               /*
> > +                * Allow re-attach for TRACING and LSM programs. If it's
> > +                * currently linked, bpf_trampoline_link_prog will fail.
> > +                * EXT programs need to specify tgt_prog_fd, so they
> > +                * re-attach in separate code path.
> > +                */
> > +               if (prog->type != BPF_PROG_TYPE_TRACING &&
> > +                   prog->type != BPF_PROG_TYPE_LSM) {
> > +                       err = -EINVAL;
> > +                       goto out_unlock;
> > +               }
> > +               btf_id = prog->aux->attach_btf_id;
> > +               key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf, btf_id);
> >         }
> >
> >         if (!prog->aux->dst_trampoline ||
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 1f3a4be4b175..48b8b9916aa2 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -437,7 +437,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
> >                 tr->extension_prog = NULL;
> >                 goto out;
> >         }
> > -       hlist_del(&prog->aux->tramp_hlist);
> > +       hlist_del_init(&prog->aux->tramp_hlist);
> 
> there is another hlist_del few lines above in error handling path of
> bpf_trampoline_link_prog(), it should probably be also updated to
> hlist_del_init(), no?

ugh, that one is missing.. will fix

thanks,
jirka

> 
> >         tr->progs_cnt[kind]--;
> >         err = bpf_trampoline_update(tr);
> >  out:
> > --
> > 2.30.2
> >
>