Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables

[Alexei Starovoitov]

On Mon, Jun 12, 2023 at 6:46 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> On Fri, 2023-06-09 at 11:15 -0700, Alexei Starovoitov wrote:
> > On Thu, Jun 8, 2023 at 5:11 PM Krister Johansen
> > <kjlx@xxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > In certain situations a program with subprograms may have a NULL
> > > extable entry.  This should not happen, and when it does, it turns
> > > a
> > > single trap into multiple.  Add a test case for further debugging
> > > and to
> > > prevent regressions.  N.b: without any other patches this can panic
> > > or
> > > oops a kernel.
> > >
> > > Signed-off-by: Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  .../bpf/prog_tests/subprogs_extable.c         | 31 +++++++++++++
> > >  .../bpf/progs/test_subprogs_extable.c         | 46
> > > +++++++++++++++++++
> > >  2 files changed, 77 insertions(+)
> > >  create mode 100644
> > > tools/testing/selftests/bpf/prog_tests/subprogs_extable.c
> > >  create mode 100644
> > > tools/testing/selftests/bpf/progs/test_subprogs_extable.c
> > >
> > > diff --git
> > > a/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c
> > > b/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c
> > > new file mode 100644
> > > index 000000000000..2201988274a4
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c
> > > @@ -0,0 +1,31 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <test_progs.h>
> > > +#include "test_subprogs_extable.skel.h"
> > > +
> > > +void test_subprogs_extable(void)
> > > +{
> > > +       const int READ_SZ = 456;
> > > +       struct test_subprogs_extable *skel;
> > > +       int err;
> > > +
> > > +       skel = test_subprogs_extable__open();
> > > +       if (!ASSERT_OK_PTR(skel, "skel_open"))
> > > +               return;
> > > +
> > > +       err = test_subprogs_extable__load(skel);
> > > +       if (!ASSERT_OK(err, "skel_load"))
> > > +               goto cleanup;
> > > +
> > > +       err = test_subprogs_extable__attach(skel);
> > > +       if (!ASSERT_OK(err, "skel_attach"))
> > > +               goto cleanup;
> > > +
> > > +       /* trigger tracepoint */
> > > +       ASSERT_OK(trigger_module_test_read(READ_SZ),
> > > "trigger_read");
> > > +
> > > +       test_subprogs_extable__detach(skel);
> > > +
> > > +cleanup:
> > > +       test_subprogs_extable__destroy(skel);
> > > +}
> > > diff --git
> > > a/tools/testing/selftests/bpf/progs/test_subprogs_extable.c
> > > b/tools/testing/selftests/bpf/progs/test_subprogs_extable.c
> > > new file mode 100644
> > > index 000000000000..c3ff66bf4cbe
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/test_subprogs_extable.c
> > > @@ -0,0 +1,46 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include "vmlinux.h"
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +
> > > +struct {
> > > +       __uint(type, BPF_MAP_TYPE_ARRAY);
> > > +       __uint(max_entries, 8);
> > > +       __type(key, __u32);
> > > +       __type(value, __u64);
> > > +} test_array SEC(".maps");
> > > +
> > > +static __u64 test_cb(struct bpf_map *map, __u32 *key, __u64 *val,
> > > void *data)
> > > +{
> > > +       return 1;
> > > +}
> > > +
> > > +SEC("fexit/bpf_testmod_return_ptr")
> > > +int BPF_PROG(handle_fexit_ret_subprogs, int arg, struct file *ret)
> > > +{
> > > +       *(volatile long *)ret;
> > > +       *(volatile int *)&ret->f_mode;
> > > +       bpf_for_each_map_elem(&test_array, test_cb, NULL, 0);
> > > +       return 0;
> > > +}
> > > +
> > > +SEC("fexit/bpf_testmod_return_ptr")
> > > +int BPF_PROG(handle_fexit_ret_subprogs2, int arg, struct file
> > > *ret)
> > > +{
> > > +       *(volatile long *)ret;
> > > +       *(volatile int *)&ret->f_mode;
> > > +       bpf_for_each_map_elem(&test_array, test_cb, NULL, 0);
> > > +       return 0;
> > > +}
> > > +
> > > +SEC("fexit/bpf_testmod_return_ptr")
> > > +int BPF_PROG(handle_fexit_ret_subprogs3, int arg, struct file
> > > *ret)
> > > +{
> > > +       *(volatile long *)ret;
> > > +       *(volatile int *)&ret->f_mode;
> > > +       bpf_for_each_map_elem(&test_array, test_cb, NULL, 0);
> > > +       return 0;
> > > +}
> >
> > What is the point of attaching 3 the same progs to the same hook?
> > One would be enough to test it, no?
> >
> > In other news...
> > Looks like this test is triggering a bug on s390.
> >
> > Ilya,
> > please take a look:
> > https://github.com/kernel-patches/bpf/actions/runs/5216942096/jobs/9416404780
> >
> > bpf_prog_78c0d4c618ed2df7_handle_fexit_ret_subprogs3
> > is crashing the kernel.
> > A bug in extable logic on s390?
>
> I think we also need this:
>
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17664,6 +17664,7 @@ static int jit_subprogs(struct bpf_verifier_env
> *env)
>         prog->bpf_func = func[0]->bpf_func;
>         prog->jited_len = func[0]->jited_len;
>         prog->aux->extable = func[0]->aux->extable;
> +       prog->aux->num_exentries = func[0]->aux->num_exentries;
>         prog->aux->func = func;
>         prog->aux->func_cnt = env->subprog_cnt;
>         bpf_prog_jit_attempt_done(prog);
>
> The reason is that s390 JIT doubles the number of extable entries due
> to how the hardware works (some exceptions point to the failing insn,
> some point to the next one).
>
> With that:
>
> Acked-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> Tested-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
>
> for the v4 series.

Great.

Krister,
could you please resubmit v5 adding the above change and Ilya's tags to patch 1?

I'd like to see green BPF CI on all platforms before landing.