From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: ardb@kernel.org, Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Alexei Starovoitov <ast@kernel.org>,
Zi Shen Lim <zlim.lnx@gmail.com>,
Andrii Nakryiko <andrii@kernel.org>, Martin Lau <kafai@fb.com>,
Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
john fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>,
andreyknvl@gmail.com, vincenzo.frascino@arm.com,
Mark Rutland <mark.rutland@arm.com>,
Sami Tolvanen <samitolvanen@google.com>,
joey.gouly@arm.com, maz@kernel.org, daizhiyuan@phytium.com.cn,
jthierry@redhat.com, Tian Tao <tiantao6@hisilicon.com>,
Peter Collingbourne <pcc@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
rppt@kernel.org, Jisheng.Zhang@synaptics.com,
liu.hailong6@zte.com.cn,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
open list <linux-kernel@vger.kernel.org>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: add exception handling selftests for tp_bpf program
Date: Thu, 4 Nov 2021 16:23:12 -0700 [thread overview]
Message-ID: <CAEf4BzaX65wMwEGZJ6HL+zOtAQjUk3A5ySiECYuSH5bLsPFAGg@mail.gmail.com> (raw)
In-Reply-To: <alpine.LRH.2.23.451.2111042248360.7576@localhost>
On Thu, Nov 4, 2021 at 3:56 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
>
>
> On Wed, 3 Nov 2021, Andrii Nakryiko wrote:
>
> > On Wed, Nov 3, 2021 at 2:50 AM Alan Maguire <alan.maguire@oracle.com> 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@oracle.com>
> > > ---
> > > 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
prev parent reply other threads:[~2021-11-04 23:23 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-03 9:49 [PATCH bpf-next 0/2] arm64/bpf: remove 128MB limit for BPF JIT programs Alan Maguire
2021-11-03 9:49 ` [PATCH bpf-next 1/2] " Alan Maguire
2021-11-03 9:49 ` [PATCH bpf-next 2/2] selftests/bpf: add exception handling selftests for tp_bpf program Alan Maguire
2021-11-03 18:39 ` Andrii Nakryiko
2021-11-04 22:56 ` Alan Maguire
2021-11-04 23:23 ` Andrii Nakryiko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzaX65wMwEGZJ6HL+zOtAQjUk3A5ySiECYuSH5bLsPFAGg@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=Jisheng.Zhang@synaptics.com \
--cc=akpm@linux-foundation.org \
--cc=alan.maguire@oracle.com \
--cc=andreyknvl@gmail.com \
--cc=andrii@kernel.org \
--cc=ardb@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=catalin.marinas@arm.com \
--cc=daizhiyuan@phytium.com.cn \
--cc=daniel@iogearbox.net \
--cc=joey.gouly@arm.com \
--cc=john.fastabend@gmail.com \
--cc=jthierry@redhat.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liu.hailong6@zte.com.cn \
--cc=mark.rutland@arm.com \
--cc=maz@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pcc@google.com \
--cc=rppt@kernel.org \
--cc=samitolvanen@google.com \
--cc=songliubraving@fb.com \
--cc=tiantao6@hisilicon.com \
--cc=vincenzo.frascino@arm.com \
--cc=will@kernel.org \
--cc=yhs@fb.com \
--cc=zlim.lnx@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).