netdev - Re: [PATCH bpf-next] selftests/bpf: Fix map

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Feb 2023 19:20:10 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next] selftests/bpf: Fix map_kptr test.

On Tue, Feb 14, 2023 at 7:02 PM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> On 02/14, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@...nel.org>
>
> > The compiler is optimizing out majority of unref_ptr read/writes, so the
> > test
> > wasn't testing much. For example, one could delete '__kptr' tag from
> > 'struct prog_test_ref_kfunc __kptr *unref_ptr;' and the test would
> > still "pass".
>
> > Convert it to volatile stores. Confirmed by comparing bpf asm
> > before/after.
>
> > Fixes: 2cbc469a6fc3 ("selftests/bpf: Add C tests for kptr")
> > Signed-off-by: Alexei Starovoitov <ast@...nel.org>
>
> Acked-by: Stanislav Fomichev <sdf@...gle.com>
>
> > ---
> >   tools/testing/selftests/bpf/progs/map_kptr.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
>
> > diff --git a/tools/testing/selftests/bpf/progs/map_kptr.c
> > b/tools/testing/selftests/bpf/progs/map_kptr.c
> > index eb8217803493..228ec45365a8 100644
> > --- a/tools/testing/selftests/bpf/progs/map_kptr.c
> > +++ b/tools/testing/selftests/bpf/progs/map_kptr.c
> > @@ -62,21 +62,23 @@ extern struct prog_test_ref_kfunc *
> >   bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int
> > b) __ksym;
> >   extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
> > __ksym;
>
>
> [..]
>
> > +#define WRITE_ONCE(x, val) ((*(volatile typeof(x) *) &(x)) = (val))
>
> (thinking out loud)
>
> Maybe time for us to put these into some common headers in the
> selftests.
> progs/test_ksyms_btf_null_check.c READ_ONCE as well..

Not quite. There is no READ_ONCE there. Only comment about it :)
But yeah a follow up is necessary, but it's not that simple.
I think it's ok to use WRITE_ONCE here, but
saying it's a generic thing for all bpf programs to use
is not something we can do without defining a BPF memory model.
So it's a whole can of worms that I'd rather not open right now.