On Thu, Oct 31, 2024 at 5:00 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> 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 <memxor@xxxxxxxxx>
> ---
> .../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
>
