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