| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Jun 2023 15:07:22 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Ilya Leoshkevich <iii@...ux.ibm.com>
Cc: Krister Johansen <kjlx@...pleofstupid.com>,
bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>,
Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>,
Stanislav Fomichev <sdf@...gle.com>,
Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf v3 2/2] selftests/bpf: add a test for subprogram extables
On Mon, Jun 12, 2023 at 6:46 AM Ilya Leoshkevich <iii@...ux.ibm.com> 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@...pleofstupid.com> 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@...pleofstupid.com>
> > > ---
> > > .../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@...ux.ibm.com>
> Tested-by: Ilya Leoshkevich <iii@...ux.ibm.com>
>
> 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.
Powered by blists - more mailing lists