On Wed, 3 Nov 2021, Andrii Nakryiko wrote:
> On Wed, Nov 3, 2021 at 2:50 AM Alan Maguire <alan.maguire@xxxxxxxxxx> 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 <alan.maguire@xxxxxxxxxx>
> > ---
> > 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
