| Message ID | 20210412162502.1417018-1-jolsa@kernel.org |
|---|---|
| Headers | show |
| Series | bpf: Tracing and lsm programs re-attach | expand |
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? > tr->progs_cnt[kind]--; > err = bpf_trampoline_update(tr); > out: > -- > 2.30.2 >
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 > > >
hi, while adding test for pinning the module while there's trampoline attach to it, I noticed that we don't allow link detach and following re-attach for trampolines. Adding that for tracing and lsm programs. You need to have patch [1] from bpf tree for test module attach test to pass. v4 changes: - fix wrong cleanup goto jump reported by Julia thanks, jirka [1] https://lore.kernel.org/bpf/20210326105900.151466-1-jolsa@kernel.org/ --- Jiri Olsa (5): bpf: Allow trampoline re-attach for tracing and lsm programs selftests/bpf: Add re-attach test to fentry_test selftests/bpf: Add re-attach test to fexit_test selftests/bpf: Add re-attach test to lsm test selftests/bpf: Test that module can't be unloaded with attached trampoline kernel/bpf/syscall.c | 23 +++++++++++++++++------ kernel/bpf/trampoline.c | 2 +- tools/testing/selftests/bpf/prog_tests/fentry_test.c | 51 ++++++++++++++++++++++++++++++++++++--------------- tools/testing/selftests/bpf/prog_tests/fexit_test.c | 51 ++++++++++++++++++++++++++++++++++++--------------- tools/testing/selftests/bpf/prog_tests/module_attach.c | 23 +++++++++++++++++++++++ tools/testing/selftests/bpf/prog_tests/test_lsm.c | 48 ++++++++++++++++++++++++++++++++++++++---------- tools/testing/selftests/bpf/test_progs.h | 2 +- 7 files changed, 152 insertions(+), 48 deletions(-)