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] [day] [month] [year] [list]

Message-ID: <CAKH8qBuO1r65-uQ2usCgsdjDHchD3Cn0g+CS7wqmvxhwbH0xEQ@mail.gmail.com>
Date:   Wed, 15 Feb 2023 09:36:12 -0800
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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:20 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> 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 :)

/* READ_ONCE */
*(volatile int *)active;
^^^ looks like a real read_once to me? not just a comment?


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

SG!