Re: [PATCH bpf-next v1 7/7] selftests/bpf: Add test for dynptr reinit in user_ringbuf callback

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, Nov 16, 2022 at 12:06:36AM IST, Joanne Koong wrote:
> On Mon, Nov 14, 2022 at 4:01 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > The original support for bpf_user_ringbuf_drain callbacks simply
> > short-circuited checks for the dynptr state, allowing users to pass
> > PTR_TO_DYNPTR (now CONST_PTR_TO_DYNPTR) to helpers that initialize a
> > dynptr. This bug would have also surfaced with other dynptr helpers in
> > the future that changed dynptr view or modified it in some way.
> >
> > Include test cases for all cases, i.e. both bpf_dynptr_from_mem and
> > bpf_ringbuf_reserve_dynptr, and ensure verifier rejects both of them.
> > Without the fix, both of these programs load and pass verification.
> >
> > Acked-by: David Vernet <void@manifault.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> Acked-by: Joanne Koong <joannelkoong@gmail.com>
>
> Left a small comment below.
>
> > ---
> >  .../selftests/bpf/prog_tests/user_ringbuf.c   |  2 ++
> >  .../selftests/bpf/progs/user_ringbuf_fail.c   | 35 +++++++++++++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> > index 39882580cb90..500a63bb70a8 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c
> > @@ -676,6 +676,8 @@ static struct {
> >         {"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"},
> >         {"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"},
> >         {"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"},
> > +       {"user_ringbuf_callback_reinit_dynptr_mem", "Dynptr has to be an uninitialized dynptr"},
> > +       {"user_ringbuf_callback_reinit_dynptr_ringbuf", "Dynptr has to be an uninitialized dynptr"},
> >  };
> >
> >  #define SUCCESS_TEST(_func) { _func, #_func }
> > diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> > index 82aba4529aa9..7730d13c0cea 100644
> > --- a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> > +++ b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c
> > @@ -18,6 +18,13 @@ struct {
> >         __uint(type, BPF_MAP_TYPE_USER_RINGBUF);
> >  } user_ringbuf SEC(".maps");
> >
> > +struct {
> > +       __uint(type, BPF_MAP_TYPE_RINGBUF);
> > +       __uint(max_entries, 2);
> > +} ringbuf SEC(".maps");
> > +
> > +static int map_value;
> > +
> >  static long
> >  bad_access1(struct bpf_dynptr *dynptr, void *context)
> >  {
> > @@ -175,3 +182,31 @@ int user_ringbuf_callback_invalid_return(void *ctx)
> >
> >         return 0;
> >  }
> > +
> > +static long
> > +try_reinit_dynptr_mem(struct bpf_dynptr *dynptr, void *context)
> > +{
> > +       bpf_dynptr_from_mem(&map_value, 4, 0, dynptr);
> > +       return 0;
> > +}
> > +
> > +static long
> > +try_reinit_dynptr_ringbuf(struct bpf_dynptr *dynptr, void *context)
> > +{
> > +       bpf_ringbuf_reserve_dynptr(&ringbuf, 8, 0, dynptr);
> > +       return 0;
> > +}
> > +
> > +SEC("?raw_tp/sys_nanosleep")
> > +int user_ringbuf_callback_reinit_dynptr_mem(void *ctx)
> > +{
> > +       bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_mem, NULL, 0);
> > +       return 0;
> > +}
> > +
> > +SEC("?raw_tp/sys_nanosleep")
>
> nit: here and above, I think this should just be "?raw_tp/" without
> the nanosleep, since there is no nanosleep tracepoint.
>

True, looks like it's the same for all prior cases in this file as well. I will
drop sys_nanosleep for all of them.