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

Commit Message

Alan Maguire Nov. 3, 2021, 9:49 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Andrii Nakryiko Nov. 3, 2021, 6:39 p.m. UTC | #1

On Wed, Nov 3, 2021 at 2:50 AM Alan Maguire <[e-mail]> wrote:
>
> 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 <[e-mail]>
> ---
>  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
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021, Oracle and/or its affiliates. */
> +
> +#include <test_progs.h>
> +
> +/* Test that verifies exception handling is working; ping to localhost
> + * will result in a receive with a NULL skb->sk; our BPF program
> + * then dereferences the an sk field which shouldn't be 0, and if we
> + * see 0 we can conclude the exception handler ran when we attempted to
> + * dereference the NULL sk and zeroed the destination register.
> + */
> +#include "exhandler_kern.skel.h"
> +
> +#define SYSTEM(...)    \
> +       (env.verbosity >= VERBOSE_VERY ?        \
> +        system(__VA_ARGS__) : system(__VA_ARGS__ " >/dev/null 2>&1"))
> +
> +void test_exhandler(void)
> +{
> +       struct exhandler_kern *skel;
> +       struct exhandler_kern__bss *bss;
> +       int err = 0, duration = 0;
> +
> +       skel = exhandler_kern__open_and_load();
> +       if (CHECK(!skel, "skel_load", "skeleton failed: %d\n", err))
> +               goto cleanup;
> +
> +       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

> +
> +       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).

> +                 "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


> +                 "verify exceptions were triggered",
> +                 "no exceptions were triggered\n"))
> +               goto cleanup;
> +cleanup:
> +       exhandler_kern__destroy(skel);
> +}
> 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

> +
> +       if (!sk && !sndbuf)
> +               exception_triggered++;
> +       return 0;
> +}
> --
> 1.8.3.1
>

Alan Maguire Nov. 4, 2021, 10:56 p.m. UTC | #2

On Wed, 3 Nov 2021, Andrii Nakryiko wrote:

> On Wed, Nov 3, 2021 at 2:50 AM Alan Maguire <[e-mail]> wrote:
> >
> > 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 <[e-mail]>
> > ---
> >  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>
> > +
> > +       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.
 
> > +
> > +       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?
 
> 
> > +                 "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.
 
> > 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

Andrii Nakryiko Nov. 4, 2021, 11:23 p.m. UTC | #3

On Thu, Nov 4, 2021 at 3:56 PM Alan Maguire <[e-mail]> wrote:
>
>
>
> On Wed, 3 Nov 2021, Andrii Nakryiko wrote:
>
> > On Wed, Nov 3, 2021 at 2:50 AM Alan Maguire <[e-mail]> wrote:
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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>
> > > +
> > > +       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.
>
> > > +
> > > +       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?

Yeah, tracing a fork would be better, thanks!. Make sure you are
filtering by pid, to avoid accidentally tripping on some unrelated
fork.

>
> >
> > > +                 "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.
>
> > > 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

Patch

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
@@ -0,0 +1,45 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021, Oracle and/or its affiliates. */
+
+#include <test_progs.h>
+
+/* Test that verifies exception handling is working; ping to localhost
+ * will result in a receive with a NULL skb->sk; our BPF program
+ * then dereferences the an sk field which shouldn't be 0, and if we
+ * see 0 we can conclude the exception handler ran when we attempted to
+ * dereference the NULL sk and zeroed the destination register.
+ */
+#include "exhandler_kern.skel.h"
+
+#define SYSTEM(...)    \
+	(env.verbosity >= VERBOSE_VERY ?        \
+	 system(__VA_ARGS__) : system(__VA_ARGS__ " >/dev/null 2>&1"))
+
+void test_exhandler(void)
+{
+	struct exhandler_kern *skel;
+	struct exhandler_kern__bss *bss;
+	int err = 0, duration = 0;
+
+	skel = exhandler_kern__open_and_load();
+	if (CHECK(!skel, "skel_load", "skeleton failed: %d\n", err))
+		goto cleanup;
+
+	bss = skel->bss;
+
+	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"),
+		  "ping localhost",
+		  "ping localhost failed\n"))
+		goto cleanup;
+
+	if (CHECK(bss->exception_triggered == 0,
+		  "verify exceptions were triggered",
+		  "no exceptions were triggered\n"))
+		goto cleanup;
+cleanup:
+	exhandler_kern__destroy(skel);
+}
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;
+	}));
+
+	if (!sk && !sndbuf)
+		exception_triggered++;
+	return 0;
+}