On Tue, Feb 14, 2023 at 7:20 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Feb 14, 2023 at 7:02 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> >
> > On 02/14, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > > 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@xxxxxxxxxx>
> >
> > Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> >
> > > ---
> > > 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!
