| Message ID | 20210203232331.2567162-3-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | BPF Ringbuffer + Sleepable Programs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: shuah@kernel.org netdev@vger.kernel.org songliubraving@fb.com linux-kselftest@vger.kernel.org john.fastabend@gmail.com kafai@fb.com yhs@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 95 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Feb 3, 2021 at 3:23 PM KP Singh <kpsingh@kernel.org> wrote: > > Instead of using shared global variables between userspace and BPF, use > the ring buffer to send the IMA hash on the BPF ring buffer. This helps > in validating both IMA and the usage of the ringbuffer in sleepable > programs. > > Signed-off-by: KP Singh <kpsingh@kernel.org> > --- > .../selftests/bpf/prog_tests/test_ima.c | 23 ++++++++++--- > tools/testing/selftests/bpf/progs/ima.c | 33 ++++++++++++++----- > 2 files changed, 43 insertions(+), 13 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c > index 61fca681d524..23fb4c9e80d1 100644 > --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c > +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c > @@ -9,6 +9,7 @@ > #include <unistd.h> > #include <sys/wait.h> > #include <test_progs.h> > +#include <linux/ring_buffer.h> > > #include "ima.skel.h" > > @@ -31,9 +32,18 @@ static int run_measured_process(const char *measured_dir, u32 *monitored_pid) > return -EINVAL; > } > > +static u64 ima_hash_from_bpf; > + > +static int process_sample(void *ctx, void *data, size_t len) > +{ > + ima_hash_from_bpf = *((u64 *)data); > + return 0; > +} > + > void test_test_ima(void) > { > char measured_dir_template[] = "/tmp/ima_measuredXXXXXX"; > + struct ring_buffer *ringbuf; > const char *measured_dir; > char cmd[256]; > > @@ -44,6 +54,11 @@ void test_test_ima(void) > if (CHECK(!skel, "skel_load", "skeleton failed\n")) > goto close_prog; > > + ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.ringbuf), > + process_sample, NULL, NULL); > + if (CHECK(!ringbuf, "ringbuf_create", "failed to create ringbuf\n")) > + goto close_prog; nit: could have used ASSERT_OK_PTR() > + > err = ima__attach(skel); > if (CHECK(err, "attach", "attach failed: %d\n", err)) > goto close_prog; > @@ -60,11 +75,9 @@ void test_test_ima(void) > if (CHECK(err, "run_measured_process", "err = %d\n", err)) > goto close_clean; > > - CHECK(skel->data->ima_hash_ret < 0, "ima_hash_ret", > - "ima_hash_ret = %ld\n", skel->data->ima_hash_ret); > - > - CHECK(skel->bss->ima_hash == 0, "ima_hash", > - "ima_hash = %lu\n", skel->bss->ima_hash); > + err = ring_buffer__poll(ringbuf, 1000); nit: given data should definitely be available here, could use ring_buffer__consume() instead and fail immediately if data is not there > + ASSERT_EQ(err, 1, "num_samples_or_err"); > + ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash"); > > close_clean: > snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir); > diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c > index 86b21aff4bc5..dd0792204a21 100644 > --- a/tools/testing/selftests/bpf/progs/ima.c > +++ b/tools/testing/selftests/bpf/progs/ima.c > @@ -9,20 +9,37 @@ > #include <bpf/bpf_helpers.h> > #include <bpf/bpf_tracing.h> > > -long ima_hash_ret = -1; > -u64 ima_hash = 0; > u32 monitored_pid = 0; > > +struct { > + __uint(type, BPF_MAP_TYPE_RINGBUF); > + __uint(max_entries, 1 << 12); > +} ringbuf SEC(".maps"); > + > char _license[] SEC("license") = "GPL"; > > SEC("lsm.s/bprm_committed_creds") > -int BPF_PROG(ima, struct linux_binprm *bprm) > +void BPF_PROG(ima, struct linux_binprm *bprm) > { > - u32 pid = bpf_get_current_pid_tgid() >> 32; > + u64 ima_hash = 0; > + u64 *sample; > + int ret; > + u32 pid; > + > + pid = bpf_get_current_pid_tgid() >> 32; > + if (pid == monitored_pid) { > + ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash, > + sizeof(ima_hash)); > + if (ret < 0 || ima_hash == 0) > + return; > + > + sample = bpf_ringbuf_reserve(&ringbuf, sizeof(u64), 0); > + if (!sample) > + return; > > - if (pid == monitored_pid) > - ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode, > - &ima_hash, sizeof(ima_hash)); > + *sample = ima_hash; > + bpf_ringbuf_submit(sample, BPF_RB_FORCE_WAKEUP); no need for BPF_RB_FORCE_WAKEUP, notification should happen deterministically. And further, if you use ring_buffer__consume() you won't even rely on notifications. Did you see any problems without this? > + } > > - return 0; > + return; > } > -- > 2.30.0.365.g02bc693789-goog >
[...] > > > > @@ -44,6 +54,11 @@ void test_test_ima(void) > > if (CHECK(!skel, "skel_load", "skeleton failed\n")) > > goto close_prog; > > > > + ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.ringbuf), > > + process_sample, NULL, NULL); > > + if (CHECK(!ringbuf, "ringbuf_create", "failed to create ringbuf\n")) > > + goto close_prog; > > nit: could have used ASSERT_OK_PTR() Updated this instance, I can separately clean some of the other places to use the ASSERT_* macros as well. > > > + > > err = ima__attach(skel); > > if (CHECK(err, "attach", "attach failed: %d\n", err)) > > goto close_prog; > > @@ -60,11 +75,9 @@ void test_test_ima(void) > > if (CHECK(err, "run_measured_process", "err = %d\n", err)) > > goto close_clean; > > > > - CHECK(skel->data->ima_hash_ret < 0, "ima_hash_ret", > > - "ima_hash_ret = %ld\n", skel->data->ima_hash_ret); > > - > > - CHECK(skel->bss->ima_hash == 0, "ima_hash", > > - "ima_hash = %lu\n", skel->bss->ima_hash); > > + err = ring_buffer__poll(ringbuf, 1000); > > nit: given data should definitely be available here, could use > ring_buffer__consume() instead and fail immediately if data is not > there Good idea. Done. > > > + ASSERT_EQ(err, 1, "num_samples_or_err"); > > + ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash"); > > > > close_clean: > > snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir); > > diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c > > index 86b21aff4bc5..dd0792204a21 100644 > > --- a/tools/testing/selftests/bpf/progs/ima.c > > +++ b/tools/testing/selftests/bpf/progs/ima.c > > @@ -9,20 +9,37 @@ > > #include <bpf/bpf_helpers.h> > > #include <bpf/bpf_tracing.h> > > > > -long ima_hash_ret = -1; > > -u64 ima_hash = 0; > > u32 monitored_pid = 0; > > > > +struct { > > + __uint(type, BPF_MAP_TYPE_RINGBUF); > > + __uint(max_entries, 1 << 12); > > +} ringbuf SEC(".maps"); [...] > > + sample = bpf_ringbuf_reserve(&ringbuf, sizeof(u64), 0); > > + if (!sample) > > + return; > > > > - if (pid == monitored_pid) > > - ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode, > > - &ima_hash, sizeof(ima_hash)); > > + *sample = ima_hash; > > + bpf_ringbuf_submit(sample, BPF_RB_FORCE_WAKEUP); > > no need for BPF_RB_FORCE_WAKEUP, notification should happen > deterministically. And further, if you use ring_buffer__consume() you > won't even rely on notifications. Did you see any problems without > this? Yes, it works without BPF_RB_FORCE_WAKEUP too, I thought I had removed it, which I clearly didn't :) > > > + } > > > > - return 0; > > + return; > > } > > -- > > 2.30.0.365.g02bc693789-goog > >
diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c index 61fca681d524..23fb4c9e80d1 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c @@ -9,6 +9,7 @@ #include <unistd.h> #include <sys/wait.h> #include <test_progs.h> +#include <linux/ring_buffer.h> #include "ima.skel.h" @@ -31,9 +32,18 @@ static int run_measured_process(const char *measured_dir, u32 *monitored_pid) return -EINVAL; } +static u64 ima_hash_from_bpf; + +static int process_sample(void *ctx, void *data, size_t len) +{ + ima_hash_from_bpf = *((u64 *)data); + return 0; +} + void test_test_ima(void) { char measured_dir_template[] = "/tmp/ima_measuredXXXXXX"; + struct ring_buffer *ringbuf; const char *measured_dir; char cmd[256]; @@ -44,6 +54,11 @@ void test_test_ima(void) if (CHECK(!skel, "skel_load", "skeleton failed\n")) goto close_prog; + ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.ringbuf), + process_sample, NULL, NULL); + if (CHECK(!ringbuf, "ringbuf_create", "failed to create ringbuf\n")) + goto close_prog; + err = ima__attach(skel); if (CHECK(err, "attach", "attach failed: %d\n", err)) goto close_prog; @@ -60,11 +75,9 @@ void test_test_ima(void) if (CHECK(err, "run_measured_process", "err = %d\n", err)) goto close_clean; - CHECK(skel->data->ima_hash_ret < 0, "ima_hash_ret", - "ima_hash_ret = %ld\n", skel->data->ima_hash_ret); - - CHECK(skel->bss->ima_hash == 0, "ima_hash", - "ima_hash = %lu\n", skel->bss->ima_hash); + err = ring_buffer__poll(ringbuf, 1000); + ASSERT_EQ(err, 1, "num_samples_or_err"); + ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash"); close_clean: snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir); diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c index 86b21aff4bc5..dd0792204a21 100644 --- a/tools/testing/selftests/bpf/progs/ima.c +++ b/tools/testing/selftests/bpf/progs/ima.c @@ -9,20 +9,37 @@ #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> -long ima_hash_ret = -1; -u64 ima_hash = 0; u32 monitored_pid = 0; +struct { + __uint(type, BPF_MAP_TYPE_RINGBUF); + __uint(max_entries, 1 << 12); +} ringbuf SEC(".maps"); + char _license[] SEC("license") = "GPL"; SEC("lsm.s/bprm_committed_creds") -int BPF_PROG(ima, struct linux_binprm *bprm) +void BPF_PROG(ima, struct linux_binprm *bprm) { - u32 pid = bpf_get_current_pid_tgid() >> 32; + u64 ima_hash = 0; + u64 *sample; + int ret; + u32 pid; + + pid = bpf_get_current_pid_tgid() >> 32; + if (pid == monitored_pid) { + ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash, + sizeof(ima_hash)); + if (ret < 0 || ima_hash == 0) + return; + + sample = bpf_ringbuf_reserve(&ringbuf, sizeof(u64), 0); + if (!sample) + return; - if (pid == monitored_pid) - ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode, - &ima_hash, sizeof(ima_hash)); + *sample = ima_hash; + bpf_ringbuf_submit(sample, BPF_RB_FORCE_WAKEUP); + } - return 0; + return; }
Instead of using shared global variables between userspace and BPF, use the ring buffer to send the IMA hash on the BPF ring buffer. This helps in validating both IMA and the usage of the ringbuffer in sleepable programs. Signed-off-by: KP Singh <kpsingh@kernel.org> --- .../selftests/bpf/prog_tests/test_ima.c | 23 ++++++++++--- tools/testing/selftests/bpf/progs/ima.c | 33 ++++++++++++++----- 2 files changed, 43 insertions(+), 13 deletions(-)