[bpf-next,v1,2/2] selftests/bpf: Add tests for raw_tp null handling

Commit Message

Kumar Kartikeya Dwivedi Nov. 1, 2024, midnight UTC

Ensure that trusted PTR_TO_BTF_ID accesses perform PROBE_MEM handling in
raw_tp program. Without the previous fix, this selftest crashes the
kernel due to a NULL-pointer dereference. Also ensure that dead code
elimination does not kick in for checks on the pointer.

Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 .../bpf/bpf_testmod/bpf_testmod-events.h      |  8 ++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  2 ++
 .../selftests/bpf/prog_tests/raw_tp_null.c    | 25 +++++++++++++++++
 .../testing/selftests/bpf/progs/raw_tp_null.c | 27 +++++++++++++++++++
 4 files changed, 62 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
 create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null.c

Comments

Andrii Nakryiko Nov. 1, 2024, 7:19 p.m. UTC | #1

On Thu, Oct 31, 2024 at 5:00 PM Kumar Kartikeya Dwivedi
<[e-mail]> wrote:
>
> Ensure that trusted PTR_TO_BTF_ID accesses perform PROBE_MEM handling in
> raw_tp program. Without the previous fix, this selftest crashes the
> kernel due to a NULL-pointer dereference. Also ensure that dead code
> elimination does not kick in for checks on the pointer.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> ---
>  .../bpf/bpf_testmod/bpf_testmod-events.h      |  8 ++++++
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  2 ++
>  .../selftests/bpf/prog_tests/raw_tp_null.c    | 25 +++++++++++++++++
>  .../testing/selftests/bpf/progs/raw_tp_null.c | 27 +++++++++++++++++++
>  4 files changed, 62 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
>  create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null.c
>
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
> index 6c3b4d4f173a..aeef86b3da74 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
> @@ -40,6 +40,14 @@ DECLARE_TRACE(bpf_testmod_test_nullable_bare,
>         TP_ARGS(ctx__nullable)
>  );
>
> +struct sk_buff;
> +
> +DECLARE_TRACE(bpf_testmod_test_raw_tp_null,
> +       TP_PROTO(struct sk_buff *skb),
> +       TP_ARGS(skb)
> +);
> +
> +
>  #undef BPF_TESTMOD_DECLARE_TRACE
>  #ifdef DECLARE_TRACE_WRITABLE
>  #define BPF_TESTMOD_DECLARE_TRACE(call, proto, args, size) \
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 8835761d9a12..4e6a9e9c0368 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -380,6 +380,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
>
>         (void)bpf_testmod_test_arg_ptr_to_struct(&struct_arg1_2);
>
> +       (void)trace_bpf_testmod_test_raw_tp_null(NULL);
> +
>         struct_arg3 = kmalloc((sizeof(struct bpf_testmod_struct_arg_3) +
>                                 sizeof(int)), GFP_KERNEL);
>         if (struct_arg3 != NULL) {
> diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
> new file mode 100644
> index 000000000000..b9068fee7d8a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> +
> +#include <test_progs.h>
> +#include "raw_tp_null.skel.h"
> +
> +void test_raw_tp_null(void)
> +{
> +       struct raw_tp_null *skel;
> +
> +       skel = raw_tp_null__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "raw_tp_null__open_and_load"))
> +               return;
> +
> +       skel->bss->tid = gettid();

this is not available everywhere, we just recently had a fix. Other
tests call syscall() directly. It might be time to add macro in one of
the helpers headers, though. But that can be done as a separate clean
up patch outside of this change (there is enough to review and
discuss)

> +
> +       if (!ASSERT_OK(raw_tp_null__attach(skel), "raw_tp_null__attach"))
> +               goto end;
> +
> +       ASSERT_OK(trigger_module_test_read(2), "trigger testmod read");
> +       ASSERT_EQ(skel->bss->i, 3, "invocations");
> +
> +end:
> +       raw_tp_null__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/raw_tp_null.c b/tools/testing/selftests/bpf/progs/raw_tp_null.c
> new file mode 100644
> index 000000000000..c7c9ad4ec3b7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/raw_tp_null.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +int tid;
> +int i;
> +
> +SEC("tp_btf/bpf_testmod_test_raw_tp_null")
> +int BPF_PROG(test_raw_tp_null, struct sk_buff *skb)
> +{
> +       if (bpf_get_current_task_btf()->pid == tid) {

nit: avoid unnecessary nesting. Check condition and return early. Also
seems nicer to have task_struct local variable for this, tbh:

struct task_struct *t = bpf_get_current_task_btf();

if (t->pid != tid)
    return 0;

/* the rest follows, unnested */

> +               i = i + skb->mark + 1;
> +
> +               /* If dead code elimination kicks in, the increment below will
> +                * be removed. For raw_tp programs, we mark input arguments as
> +                * PTR_MAYBE_NULL, so branch prediction should never kick in.
> +                */
> +               if (!skb)
> +                       i += 2;
> +       }
> +
> +       return 0;
> +}
> --
> 2.43.5
>

Kumar Kartikeya Dwivedi Nov. 3, 2024, 3:58 p.m. UTC | #2

On Fri, 1 Nov 2024 at 14:19, Andrii Nakryiko <[e-mail]> wrote:
>
> On Thu, Oct 31, 2024 at 5:00 PM Kumar Kartikeya Dwivedi
> <[e-mail]> wrote:
> >
> > Ensure that trusted PTR_TO_BTF_ID accesses perform PROBE_MEM handling in
> > raw_tp program. Without the previous fix, this selftest crashes the
> > kernel due to a NULL-pointer dereference. Also ensure that dead code
> > elimination does not kick in for checks on the pointer.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> > ---
> >  .../bpf/bpf_testmod/bpf_testmod-events.h      |  8 ++++++
> >  .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  2 ++
> >  .../selftests/bpf/prog_tests/raw_tp_null.c    | 25 +++++++++++++++++
> >  .../testing/selftests/bpf/progs/raw_tp_null.c | 27 +++++++++++++++++++
> >  4 files changed, 62 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null.c
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
> > index 6c3b4d4f173a..aeef86b3da74 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
> > @@ -40,6 +40,14 @@ DECLARE_TRACE(bpf_testmod_test_nullable_bare,
> >         TP_ARGS(ctx__nullable)
> >  );
> >
> > +struct sk_buff;
> > +
> > +DECLARE_TRACE(bpf_testmod_test_raw_tp_null,
> > +       TP_PROTO(struct sk_buff *skb),
> > +       TP_ARGS(skb)
> > +);
> > +
> > +
> >  #undef BPF_TESTMOD_DECLARE_TRACE
> >  #ifdef DECLARE_TRACE_WRITABLE
> >  #define BPF_TESTMOD_DECLARE_TRACE(call, proto, args, size) \
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index 8835761d9a12..4e6a9e9c0368 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -380,6 +380,8 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
> >
> >         (void)bpf_testmod_test_arg_ptr_to_struct(&struct_arg1_2);
> >
> > +       (void)trace_bpf_testmod_test_raw_tp_null(NULL);
> > +
> >         struct_arg3 = kmalloc((sizeof(struct bpf_testmod_struct_arg_3) +
> >                                 sizeof(int)), GFP_KERNEL);
> >         if (struct_arg3 != NULL) {
> > diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
> > new file mode 100644
> > index 000000000000..b9068fee7d8a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> > +
> > +#include <test_progs.h>
> > +#include "raw_tp_null.skel.h"
> > +
> > +void test_raw_tp_null(void)
> > +{
> > +       struct raw_tp_null *skel;
> > +
> > +       skel = raw_tp_null__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "raw_tp_null__open_and_load"))
> > +               return;
> > +
> > +       skel->bss->tid = gettid();
>
> this is not available everywhere, we just recently had a fix. Other
> tests call syscall() directly. It might be time to add macro in one of
> the helpers headers, though. But that can be done as a separate clean
> up patch outside of this change (there is enough to review and
> discuss)

Ok, I can include a clean up patch in the next revision.

>
> > +
> > +       if (!ASSERT_OK(raw_tp_null__attach(skel), "raw_tp_null__attach"))
> > +               goto end;
> > +
> > +       ASSERT_OK(trigger_module_test_read(2), "trigger testmod read");
> > +       ASSERT_EQ(skel->bss->i, 3, "invocations");
> > +
> > +end:
> > +       raw_tp_null__destroy(skel);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/raw_tp_null.c b/tools/testing/selftests/bpf/progs/raw_tp_null.c
> > new file mode 100644
> > index 000000000000..c7c9ad4ec3b7
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/raw_tp_null.c
> > @@ -0,0 +1,27 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> > +
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +int tid;
> > +int i;
> > +
> > +SEC("tp_btf/bpf_testmod_test_raw_tp_null")
> > +int BPF_PROG(test_raw_tp_null, struct sk_buff *skb)
> > +{
> > +       if (bpf_get_current_task_btf()->pid == tid) {
>
> nit: avoid unnecessary nesting. Check condition and return early. Also
> seems nicer to have task_struct local variable for this, tbh:
>
> struct task_struct *t = bpf_get_current_task_btf();
>
> if (t->pid != tid)
>     return 0;
>
> /* the rest follows, unnested */

Ack.

>
> > +               i = i + skb->mark + 1;
> > +
> > +               /* If dead code elimination kicks in, the increment below will
> > +                * be removed. For raw_tp programs, we mark input arguments as
> > +                * PTR_MAYBE_NULL, so branch prediction should never kick in.
> > +                */
> > +               if (!skb)
> > +                       i += 2;
> > +       }
> > +
> > +       return 0;
> > +}
> > --
> > 2.43.5
> >

Patch

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
index 6c3b4d4f173a..aeef86b3da74 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod-events.h
@@ -40,6 +40,14 @@  DECLARE_TRACE(bpf_testmod_test_nullable_bare,
 	TP_ARGS(ctx__nullable)
 );
 
+struct sk_buff;
+
+DECLARE_TRACE(bpf_testmod_test_raw_tp_null,
+	TP_PROTO(struct sk_buff *skb),
+	TP_ARGS(skb)
+);
+
+
 #undef BPF_TESTMOD_DECLARE_TRACE
 #ifdef DECLARE_TRACE_WRITABLE
 #define BPF_TESTMOD_DECLARE_TRACE(call, proto, args, size) \
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 8835761d9a12..4e6a9e9c0368 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -380,6 +380,8 @@  bpf_testmod_test_read(struct file *file, struct kobject *kobj,
 
 	(void)bpf_testmod_test_arg_ptr_to_struct(&struct_arg1_2);
 
+	(void)trace_bpf_testmod_test_raw_tp_null(NULL);
+
 	struct_arg3 = kmalloc((sizeof(struct bpf_testmod_struct_arg_3) +
 				sizeof(int)), GFP_KERNEL);
 	if (struct_arg3 != NULL) {
diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
new file mode 100644
index 000000000000..b9068fee7d8a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c
@@ -0,0 +1,25 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <test_progs.h>
+#include "raw_tp_null.skel.h"
+
+void test_raw_tp_null(void)
+{
+	struct raw_tp_null *skel;
+
+	skel = raw_tp_null__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "raw_tp_null__open_and_load"))
+		return;
+
+	skel->bss->tid = gettid();
+
+	if (!ASSERT_OK(raw_tp_null__attach(skel), "raw_tp_null__attach"))
+		goto end;
+
+	ASSERT_OK(trigger_module_test_read(2), "trigger testmod read");
+	ASSERT_EQ(skel->bss->i, 3, "invocations");
+
+end:
+	raw_tp_null__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/raw_tp_null.c b/tools/testing/selftests/bpf/progs/raw_tp_null.c
new file mode 100644
index 000000000000..c7c9ad4ec3b7
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/raw_tp_null.c
@@ -0,0 +1,27 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+int tid;
+int i;
+
+SEC("tp_btf/bpf_testmod_test_raw_tp_null")
+int BPF_PROG(test_raw_tp_null, struct sk_buff *skb)
+{
+	if (bpf_get_current_task_btf()->pid == tid) {
+		i = i + skb->mark + 1;
+
+		/* If dead code elimination kicks in, the increment below will
+		 * be removed. For raw_tp programs, we mark input arguments as
+		 * PTR_MAYBE_NULL, so branch prediction should never kick in.
+		 */
+		if (!skb)
+			i += 2;
+	}
+
+	return 0;
+}