| Message ID | 20210406212913.970917-1-jolsa@kernel.org |
|---|---|
| Headers | show |
| Series | bpf: Tracing and lsm programs re-attach | expand |
Jiri Olsa <jolsa@kernel.org> writes: > 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. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
On Wed, Apr 7, 2021 at 4:21 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding the test to re-attach (detach/attach again) tracing > fexit programs, plus check that already linked program can't > be attached again. > > Fixing the number of check-ed results, which should be 8. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > .../selftests/bpf/prog_tests/fexit_test.c | 48 +++++++++++++++---- > 1 file changed, 38 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_test.c b/tools/testing/selftests/bpf/prog_tests/fexit_test.c > index 78d7a2765c27..579e620e6612 100644 > --- a/tools/testing/selftests/bpf/prog_tests/fexit_test.c > +++ b/tools/testing/selftests/bpf/prog_tests/fexit_test.c > @@ -3,20 +3,24 @@ > #include <test_progs.h> > #include "fexit_test.skel.h" > > -void test_fexit_test(void) > +static __u32 duration; > + > +static int fexit_test(struct fexit_test *fexit_skel) > { > - struct fexit_test *fexit_skel = NULL; > + struct bpf_link *link; > int err, prog_fd, i; > - __u32 duration = 0, retval; > __u64 *result; > - > - fexit_skel = fexit_test__open_and_load(); > - if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n")) > - goto cleanup; > + __u32 retval; > > err = fexit_test__attach(fexit_skel); > if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err)) > - goto cleanup; > + return err; > + > + /* Check that already linked program can't be attached again. */ > + link = bpf_program__attach(fexit_skel->progs.test1); > + if (CHECK(!IS_ERR(link), "fexit_attach_link", > + "re-attach without detach should not succeed")) > + return -1; > > prog_fd = bpf_program__fd(fexit_skel->progs.test1); > err = bpf_prog_test_run(prog_fd, 1, NULL, 0, > @@ -26,12 +30,36 @@ void test_fexit_test(void) > err, errno, retval, duration); > > result = (__u64 *)fexit_skel->bss; > - for (i = 0; i < 6; i++) { > + for (i = 0; i < 8; i++) { > if (CHECK(result[i] != 1, "result", > "fexit_test%d failed err %lld\n", i + 1, result[i])) > - goto cleanup; > + return -1; > } > > + fexit_test__detach(fexit_skel); > + > + /* zero results for re-attach test */ > + for (i = 0; i < 8; i++) > + result[i] = 0; memset(fexit_skel->bss, 0, sizeof(*fexit_skel->bss)) ? ;) and see my nits in previous patch about ASSERT over CHECK > + return 0; > +} > + > +void test_fexit_test(void) > +{ > + struct fexit_test *fexit_skel = NULL; > + int err; > + > + fexit_skel = fexit_test__open_and_load(); > + if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n")) > + goto cleanup; > + > + err = fexit_test(fexit_skel); > + if (CHECK(err, "fexit_test", "first attach failed\n")) > + goto cleanup; > + > + err = fexit_test(fexit_skel); > + CHECK(err, "fexit_test", "second attach failed\n"); > + > cleanup: > fexit_test__destroy(fexit_skel); > } > -- > 2.30.2 >
On Wed, Apr 7, 2021 at 4:22 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding test to verify that once we attach module's trampoline, > the module can't be unloaded. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- To be fair, to test that you are actually testing what you think you are testing, you'd have to prove that you *can* detach when no program is attached to bpf_testmod ;) You'd also need kern_sync_rcu() to wait for all the async clean up to complete inside the kernel. But that doesn't interact with other tests well, so I think it's fine. grumpily due to CHECK() usage (please do consider updating to ASSERT): Acked-by: Andrii Nakryiko <andrii@kernel.org> > .../selftests/bpf/prog_tests/module_attach.c | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c > index 5bc53d53d86e..d180b8c28287 100644 > --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c > +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c > @@ -45,12 +45,18 @@ static int trigger_module_test_write(int write_sz) > return 0; > } > > +static int delete_module(const char *name, int flags) > +{ > + return syscall(__NR_delete_module, name, flags); > +} > + > void test_module_attach(void) > { > const int READ_SZ = 456; > const int WRITE_SZ = 457; > struct test_module_attach* skel; > struct test_module_attach__bss *bss; > + struct bpf_link *link; > int err; > > skel = test_module_attach__open(); > @@ -84,6 +90,23 @@ void test_module_attach(void) > ASSERT_EQ(bss->fexit_ret, -EIO, "fexit_tet"); > ASSERT_EQ(bss->fmod_ret_read_sz, READ_SZ, "fmod_ret"); > > + test_module_attach__detach(skel); > + > + /* attach fentry/fexit and make sure it get's module reference */ > + link = bpf_program__attach(skel->progs.handle_fentry); > + if (CHECK(IS_ERR(link), "attach_fentry", "err: %ld\n", PTR_ERR(link))) > + goto cleanup; > + > + ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module"); > + bpf_link__destroy(link); > + > + link = bpf_program__attach(skel->progs.handle_fexit); > + if (CHECK(IS_ERR(link), "attach_fexit", "err: %ld\n", PTR_ERR(link))) > + goto cleanup; > + > + ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module"); > + bpf_link__destroy(link); > + > cleanup: > test_module_attach__destroy(skel); > } > -- > 2.30.2 >
On Wed, Apr 07, 2021 at 03:51:46PM -0700, Andrii Nakryiko wrote: > On Wed, Apr 7, 2021 at 4:21 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Adding the test to re-attach (detach/attach again) tracing > > fexit programs, plus check that already linked program can't > > be attached again. > > > > Fixing the number of check-ed results, which should be 8. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > .../selftests/bpf/prog_tests/fexit_test.c | 48 +++++++++++++++---- > > 1 file changed, 38 insertions(+), 10 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_test.c b/tools/testing/selftests/bpf/prog_tests/fexit_test.c > > index 78d7a2765c27..579e620e6612 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/fexit_test.c > > +++ b/tools/testing/selftests/bpf/prog_tests/fexit_test.c > > @@ -3,20 +3,24 @@ > > #include <test_progs.h> > > #include "fexit_test.skel.h" > > > > -void test_fexit_test(void) > > +static __u32 duration; > > + > > +static int fexit_test(struct fexit_test *fexit_skel) > > { > > - struct fexit_test *fexit_skel = NULL; > > + struct bpf_link *link; > > int err, prog_fd, i; > > - __u32 duration = 0, retval; > > __u64 *result; > > - > > - fexit_skel = fexit_test__open_and_load(); > > - if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n")) > > - goto cleanup; > > + __u32 retval; > > > > err = fexit_test__attach(fexit_skel); > > if (CHECK(err, "fexit_attach", "fexit attach failed: %d\n", err)) > > - goto cleanup; > > + return err; > > + > > + /* Check that already linked program can't be attached again. */ > > + link = bpf_program__attach(fexit_skel->progs.test1); > > + if (CHECK(!IS_ERR(link), "fexit_attach_link", > > + "re-attach without detach should not succeed")) > > + return -1; > > > > prog_fd = bpf_program__fd(fexit_skel->progs.test1); > > err = bpf_prog_test_run(prog_fd, 1, NULL, 0, > > @@ -26,12 +30,36 @@ void test_fexit_test(void) > > err, errno, retval, duration); > > > > result = (__u64 *)fexit_skel->bss; > > - for (i = 0; i < 6; i++) { > > + for (i = 0; i < 8; i++) { > > if (CHECK(result[i] != 1, "result", > > "fexit_test%d failed err %lld\n", i + 1, result[i])) > > - goto cleanup; > > + return -1; > > } > > > > + fexit_test__detach(fexit_skel); > > + > > + /* zero results for re-attach test */ > > + for (i = 0; i < 8; i++) > > + result[i] = 0; > > memset(fexit_skel->bss, 0, sizeof(*fexit_skel->bss)) ? ;) > > and see my nits in previous patch about ASSERT over CHECK sure ;-) thanks jirka
On Wed, Apr 07, 2021 at 04:04:48PM -0700, Andrii Nakryiko wrote: > On Wed, Apr 7, 2021 at 4:22 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > > Adding test to verify that once we attach module's trampoline, > > the module can't be unloaded. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > To be fair, to test that you are actually testing what you think you > are testing, you'd have to prove that you *can* detach when no program > is attached to bpf_testmod ;) You'd also need kern_sync_rcu() to wait > for all the async clean up to complete inside the kernel. But that > doesn't interact with other tests well, so I think it's fine. well without the kernel change the module gets unloaded and the test below fails.. we could add module unload test, but as you described it could probably interfere with other tests > > grumpily due to CHECK() usage (please do consider updating to ASSERT): ok, will check thanks, jirka > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > .../selftests/bpf/prog_tests/module_attach.c | 23 +++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c > > index 5bc53d53d86e..d180b8c28287 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c > > +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c > > @@ -45,12 +45,18 @@ static int trigger_module_test_write(int write_sz) > > return 0; > > } > > > > +static int delete_module(const char *name, int flags) > > +{ > > + return syscall(__NR_delete_module, name, flags); > > +} > > + > > void test_module_attach(void) > > { > > const int READ_SZ = 456; > > const int WRITE_SZ = 457; > > struct test_module_attach* skel; > > struct test_module_attach__bss *bss; > > + struct bpf_link *link; > > int err; > > > > skel = test_module_attach__open(); > > @@ -84,6 +90,23 @@ void test_module_attach(void) > > ASSERT_EQ(bss->fexit_ret, -EIO, "fexit_tet"); > > ASSERT_EQ(bss->fmod_ret_read_sz, READ_SZ, "fmod_ret"); > > > > + test_module_attach__detach(skel); > > + > > + /* attach fentry/fexit and make sure it get's module reference */ > > + link = bpf_program__attach(skel->progs.handle_fentry); > > + if (CHECK(IS_ERR(link), "attach_fentry", "err: %ld\n", PTR_ERR(link))) > > + goto cleanup; > > + > > + ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module"); > > + bpf_link__destroy(link); > > + > > + link = bpf_program__attach(skel->progs.handle_fexit); > > + if (CHECK(IS_ERR(link), "attach_fexit", "err: %ld\n", PTR_ERR(link))) > > + goto cleanup; > > + > > + ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module"); > > + bpf_link__destroy(link); > > + > > cleanup: > > test_module_attach__destroy(skel); > > } > > -- > > 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. v2 changes: - allow re-attach for TRACING and LSM programs - add lsm re-attach test 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 | 48 ++++++++++++++++++++++++++++++++++++++---------- tools/testing/selftests/bpf/prog_tests/fexit_test.c | 48 ++++++++++++++++++++++++++++++++++++++---------- tools/testing/selftests/bpf/prog_tests/module_attach.c | 23 +++++++++++++++++++++++ tools/testing/selftests/bpf/prog_tests/test_lsm.c | 48 ++++++++++++++++++++++++++++++++++++++---------- 6 files changed, 155 insertions(+), 37 deletions(-)