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

[Andrii Nakryiko]

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
> @@ -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
>