* [PATCH bpf-next 0/2] BPF Ringbuffer + Sleepable Programs
@ 2021-02-03 23:23 KP Singh
2021-02-03 23:23 ` [PATCH bpf-next 1/2] bpf: Allow usage of BPF ringbuffer in sleepable programs KP Singh
2021-02-03 23:23 ` [PATCH bpf-next 2/2] bpf/selftests: Update the IMA test to use BPF ring buffer KP Singh
0 siblings, 2 replies; 6+ messages in thread
From: KP Singh @ 2021-02-03 23:23 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
Sleepable programs currently do not have access to any ringbuffer and
since the perf ring buffer is a per-cpu map, it would not be trivial to
enable for sleepable programs. Our specific use-case is to use the
bpf_ima_inode_hash helper and write the hash to a ring buffer from a
sleepable LSM hook.
This series allows the BPF ringbuffer to be used in sleepable programs
(tracing and lsm). Since the helper prototypes were already exposed
the only change required was have the verifier allow
BPF_MAP_TYPE_RINGBUF for sleepable programs. The ima test is also
modified to use the ringbuffer instead of global variables.
Based on dicussions we had over the BPF office hours and enabling all
the possible debug options, I could not find any issues or warnings when
using the ring buffer from sleepable programs.
KP Singh (2):
bpf: Allow usage of BPF ringbuffer in sleepable programs
bpf/selftests: Update the IMA test to use BPF ring buffer
kernel/bpf/verifier.c | 2 ++
.../selftests/bpf/prog_tests/test_ima.c | 23 ++++++++++---
tools/testing/selftests/bpf/progs/ima.c | 33 ++++++++++++++-----
3 files changed, 45 insertions(+), 13 deletions(-)
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/2] bpf: Allow usage of BPF ringbuffer in sleepable programs
2021-02-03 23:23 [PATCH bpf-next 0/2] BPF Ringbuffer + Sleepable Programs KP Singh
@ 2021-02-03 23:23 ` KP Singh
2021-02-04 5:02 ` Andrii Nakryiko
2021-02-03 23:23 ` [PATCH bpf-next 2/2] bpf/selftests: Update the IMA test to use BPF ring buffer KP Singh
1 sibling, 1 reply; 6+ messages in thread
From: KP Singh @ 2021-02-03 23:23 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
The BPF ringbuffer map is pre-allocated and the implementation logic
does not rely on disabling preemption or per-cpu data structures. Using
the BPF ringbuffer sleepable LSM and tracing programs does not trigger
any warnings with DEBUG_ATOMIC_SLEEP, DEBUG_PREEMPT,
PROVE_RCU and PROVE_LOCKING and LOCKDEP enabled.
This allows helpers like bpf_copy_from_user and bpf_ima_inode_hash to
write to the BPF ring buffer from sleepable BPF programs.
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
kernel/bpf/verifier.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5e09632efddb..4c33b4840438 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10024,6 +10024,8 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
return -EINVAL;
}
break;
+ case BPF_MAP_TYPE_RINGBUF:
+ break;
default:
verbose(env,
"Sleepable programs can only use array and hash maps\n");
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] bpf/selftests: Update the IMA test to use BPF ring buffer
2021-02-03 23:23 [PATCH bpf-next 0/2] BPF Ringbuffer + Sleepable Programs KP Singh
2021-02-03 23:23 ` [PATCH bpf-next 1/2] bpf: Allow usage of BPF ringbuffer in sleepable programs KP Singh
@ 2021-02-03 23:23 ` KP Singh
2021-02-04 4:59 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: KP Singh @ 2021-02-03 23:23 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
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;
+
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;
}
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf/selftests: Update the IMA test to use BPF ring buffer
2021-02-03 23:23 ` [PATCH bpf-next 2/2] bpf/selftests: Update the IMA test to use BPF ring buffer KP Singh
@ 2021-02-04 4:59 ` Andrii Nakryiko
2021-02-04 18:46 ` KP Singh
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2021-02-04 4:59 UTC (permalink / raw)
To: KP Singh
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Allow usage of BPF ringbuffer in sleepable programs
2021-02-03 23:23 ` [PATCH bpf-next 1/2] bpf: Allow usage of BPF ringbuffer in sleepable programs KP Singh
@ 2021-02-04 5:02 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2021-02-04 5:02 UTC (permalink / raw)
To: KP Singh
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
On Wed, Feb 3, 2021 at 3:23 PM KP Singh <kpsingh@kernel.org> wrote:
>
> The BPF ringbuffer map is pre-allocated and the implementation logic
> does not rely on disabling preemption or per-cpu data structures. Using
> the BPF ringbuffer sleepable LSM and tracing programs does not trigger
> any warnings with DEBUG_ATOMIC_SLEEP, DEBUG_PREEMPT,
> PROVE_RCU and PROVE_LOCKING and LOCKDEP enabled.
>
> This allows helpers like bpf_copy_from_user and bpf_ima_inode_hash to
> write to the BPF ring buffer from sleepable BPF programs.
>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
Yes, I believe ringbuf is ready for sleepable BPF as is. Its commit()
implementation is racing with other CPUs anyways, so it doesn't matter
if reserve and commit happen on the same CPU or different ones.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> kernel/bpf/verifier.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5e09632efddb..4c33b4840438 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10024,6 +10024,8 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> return -EINVAL;
> }
> break;
> + case BPF_MAP_TYPE_RINGBUF:
> + break;
> default:
> verbose(env,
> "Sleepable programs can only use array and hash maps\n");
> --
> 2.30.0.365.g02bc693789-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] bpf/selftests: Update the IMA test to use BPF ring buffer
2021-02-04 4:59 ` Andrii Nakryiko
@ 2021-02-04 18:46 ` KP Singh
0 siblings, 0 replies; 6+ messages in thread
From: KP Singh @ 2021-02-04 18:46 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Florent Revest, Brendan Jackman
[...]
> >
> > @@ -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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-04 18:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 23:23 [PATCH bpf-next 0/2] BPF Ringbuffer + Sleepable Programs KP Singh
2021-02-03 23:23 ` [PATCH bpf-next 1/2] bpf: Allow usage of BPF ringbuffer in sleepable programs KP Singh
2021-02-04 5:02 ` Andrii Nakryiko
2021-02-03 23:23 ` [PATCH bpf-next 2/2] bpf/selftests: Update the IMA test to use BPF ring buffer KP Singh
2021-02-04 4:59 ` Andrii Nakryiko
2021-02-04 18:46 ` KP Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).