| Message ID | 20230214235051.22938-1-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 62d101d5f422cde39b269f7eb4cbbe2f1e26f9d4 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Fix map_kptr test. | expand |
On 02/14, Alexei Starovoitov wrote: > From: Alexei Starovoitov <ast@kernel.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@kernel.org> Acked-by: Stanislav Fomichev <sdf@google.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.. > + > static void test_kptr_unref(struct map_value *v) > { > struct prog_test_ref_kfunc *p; > p = v->unref_ptr; > /* store untrusted_ptr_or_null_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > if (!p) > return; > if (p->a + p->b > 100) > return; > /* store untrusted_ptr_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > /* store NULL */ > - v->unref_ptr = NULL; > + WRITE_ONCE(v->unref_ptr, NULL); > } > static void test_kptr_ref(struct map_value *v) > @@ -85,7 +87,7 @@ static void test_kptr_ref(struct map_value *v) > p = v->ref_ptr; > /* store ptr_or_null_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > if (!p) > return; > if (p->a + p->b > 100) > @@ -99,7 +101,7 @@ static void test_kptr_ref(struct map_value *v) > return; > } > /* store ptr_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > bpf_kfunc_call_test_release(p); > p = bpf_kfunc_call_test_acquire(&(unsigned long){0}); > -- > 2.30.2
On Tue, Feb 14, 2023 at 7:02 PM Stanislav Fomichev <sdf@google.com> wrote: > > On 02/14, Alexei Starovoitov wrote: > > From: Alexei Starovoitov <ast@kernel.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@kernel.org> > > Acked-by: Stanislav Fomichev <sdf@google.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.
On Wed, Feb 15, 2023 at 12:50:51AM CET, Alexei Starovoitov wrote: > From: Alexei Starovoitov <ast@kernel.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@kernel.org> > --- Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> There's also the same test in the test_verifier suite, so there's still coverage for this case. > 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)) > + > static void test_kptr_unref(struct map_value *v) > { > struct prog_test_ref_kfunc *p; > > p = v->unref_ptr; > /* store untrusted_ptr_or_null_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > if (!p) > return; > if (p->a + p->b > 100) > return; > /* store untrusted_ptr_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > /* store NULL */ > - v->unref_ptr = NULL; > + WRITE_ONCE(v->unref_ptr, NULL); > } > > static void test_kptr_ref(struct map_value *v) > @@ -85,7 +87,7 @@ static void test_kptr_ref(struct map_value *v) > > p = v->ref_ptr; > /* store ptr_or_null_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > if (!p) > return; > if (p->a + p->b > 100) > @@ -99,7 +101,7 @@ static void test_kptr_ref(struct map_value *v) > return; > } > /* store ptr_ */ > - v->unref_ptr = p; > + WRITE_ONCE(v->unref_ptr, p); > bpf_kfunc_call_test_release(p); > > p = bpf_kfunc_call_test_acquire(&(unsigned long){0}); > -- > 2.30.2 >
Hello: This patch was applied to bpf/bpf-next.git (master) by Martin KaFai Lau <martin.lau@kernel.org>: On Tue, 14 Feb 2023 15:50:51 -0800 you wrote: > From: Alexei Starovoitov <ast@kernel.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. > > [...] Here is the summary with links: - [bpf-next] selftests/bpf: Fix map_kptr test. https://git.kernel.org/bpf/bpf-next/c/62d101d5f422 You are awesome, thank you!
On Tue, Feb 14, 2023 at 7:20 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Feb 14, 2023 at 7:02 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > On 02/14, Alexei Starovoitov wrote: > > > From: Alexei Starovoitov <ast@kernel.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@kernel.org> > > > > Acked-by: Stanislav Fomichev <sdf@google.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!
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)) + static void test_kptr_unref(struct map_value *v) { struct prog_test_ref_kfunc *p; p = v->unref_ptr; /* store untrusted_ptr_or_null_ */ - v->unref_ptr = p; + WRITE_ONCE(v->unref_ptr, p); if (!p) return; if (p->a + p->b > 100) return; /* store untrusted_ptr_ */ - v->unref_ptr = p; + WRITE_ONCE(v->unref_ptr, p); /* store NULL */ - v->unref_ptr = NULL; + WRITE_ONCE(v->unref_ptr, NULL); } static void test_kptr_ref(struct map_value *v) @@ -85,7 +87,7 @@ static void test_kptr_ref(struct map_value *v) p = v->ref_ptr; /* store ptr_or_null_ */ - v->unref_ptr = p; + WRITE_ONCE(v->unref_ptr, p); if (!p) return; if (p->a + p->b > 100) @@ -99,7 +101,7 @@ static void test_kptr_ref(struct map_value *v) return; } /* store ptr_ */ - v->unref_ptr = p; + WRITE_ONCE(v->unref_ptr, p); bpf_kfunc_call_test_release(p); p = bpf_kfunc_call_test_acquire(&(unsigned long){0});