Re: [PATCH bpf-next 2/2] selftests/bpf: add exception handling selftests for tp_bpf program

On Wed, 3 Nov 2021, Andrii Nakryiko wrote:

On Wed, Nov 3, 2021 at 2:50 AM Alan Maguire [off-list ref] wrote:

quoted

Exception handling is triggered in BPF tracing programs when
a NULL pointer is dereferenced; the exception handler zeroes the
target register and execution of the BPF program progresses.

To test exception handling then, we need to trigger a NULL pointer
dereference for a field which should never be zero; if it is, the
only explanation is the exception handler ran.  The skb->sk is
the NULL pointer chosen (for a ping received for 127.0.0.1 there
is no associated socket), and the sk_sndbuf size is chosen as the
"should never be 0" field.  Test verifies sk is NULL and sk_sndbuf
is zero.

Signed-off-by: Alan Maguire <redacted>
---
 tools/testing/selftests/bpf/prog_tests/exhandler.c | 45 ++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/exhandler_kern.c | 35 +++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/exhandler.c
 create mode 100644 tools/testing/selftests/bpf/progs/exhandler_kern.c

diff --git a/tools/testing/selftests/bpf/prog_tests/exhandler.c b/tools/testing/selftests/bpf/prog_tests/exhandler.c
new file mode 100644
index 0000000..5999498
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/exhandler.c

<snip>

quoted

+
+       bss = skel->bss;

nit: you don't need to have a separate variable for that,
skel->bss->exception_triggered in below check would be just as
readable

sure, will do.
 

quoted

+
+       err = exhandler_kern__attach(skel);
+       if (CHECK(err, "attach", "attach failed: %d\n", err))
+               goto cleanup;
+
+       if (CHECK(SYSTEM("ping -c 1 127.0.0.1"),

Is there some other tracepoint or kernel function that could be used
for testing and triggered without shelling out to ping binary? This
hurts test isolation and will make it or some other ping-using
selftests spuriously fail when running in parallel test mode (i.e.,
sudo ./test_progs -j).

I've got a new version of this working which uses a fork() in
combination with tp_btf/task_newtask ; the new task will have
a NULL task->task_works pointer, but if it wasn't NULL it
would have to point at a struct callback_head containing a
non-NULL callback function. So we can verify that
task->task_works and task->task_works->func are NULL to ensure
exception triggered instead.  That should interfere
less with other parallel tests hopefully?
 

quoted

+                 "ping localhost",
+                 "ping localhost failed\n"))
+               goto cleanup;
+
+       if (CHECK(bss->exception_triggered == 0,

please use ASSERT_EQ() instead, CHECK()s are kind of deprecated for new tests

sure, will do.
 

quoted

diff --git a/tools/testing/selftests/bpf/progs/exhandler_kern.c b/tools/testing/selftests/bpf/progs/exhandler_kern.c
new file mode 100644
index 0000000..4049450
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/exhandler_kern.c

@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021, Oracle and/or its affiliates. */
+
+#include "vmlinux.h"
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+
+char _license[] SEC("license") = "GPL";
+
+unsigned int exception_triggered;
+
+/* TRACE_EVENT(netif_rx,
+ *         TP_PROTO(struct sk_buff *skb),
+ */
+SEC("tp_btf/netif_rx")
+int BPF_PROG(trace_netif_rx, struct sk_buff *skb)
+{
+       struct sock *sk;
+       int sndbuf;
+
+       /* To verify we hit an exception we dereference skb->sk->sk_sndbuf;
+        * sndbuf size should never be zero, so if it is we know the exception
+        * handler triggered and zeroed the destination register.
+        */
+       __builtin_preserve_access_index(({
+               sk = skb->sk;
+               sndbuf = sk->sk_sndbuf;
+       }));

you don't need __builtin_preserve_access_index(({ }) region, because
vmlinux.h already annotates all the types with preserve_access_index
attribute

ah, great, I missed that somehow. Thanks! 

Alan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel