Re: [PATCH bpf-next v6 12/13] selftests/bpf: Add verifier tests for kptr

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

On Tue, Apr 26, 2022 at 09:05:44AM IST, Alexei Starovoitov wrote:
> On Mon, Apr 25, 2022 at 03:19:00AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Reuse bpf_prog_test functions to test the support for PTR_TO_BTF_ID in
> > BPF map case, including some tests that verify implementation sanity and
> > corner cases.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  net/bpf/test_run.c                            |  45 +-
> >  tools/testing/selftests/bpf/test_verifier.c   |  55 +-
> >  .../testing/selftests/bpf/verifier/map_kptr.c | 469 ++++++++++++++++++
> >  3 files changed, 562 insertions(+), 7 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/verifier/map_kptr.c
> >
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index e7b9c2636d10..29fe32821e7e 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -584,6 +584,12 @@ noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
> >  {
> >  }
> >
> > +noinline struct prog_test_ref_kfunc *
> > +bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b)
> > +{
> > +	return &prog_test_struct;
> > +}
> > +
> >  struct prog_test_pass1 {
> >  	int x0;
> >  	struct {
> > @@ -669,6 +675,7 @@ BTF_ID(func, bpf_kfunc_call_test3)
> >  BTF_ID(func, bpf_kfunc_call_test_acquire)
> >  BTF_ID(func, bpf_kfunc_call_test_release)
> >  BTF_ID(func, bpf_kfunc_call_memb_release)
> > +BTF_ID(func, bpf_kfunc_call_test_kptr_get)
> >  BTF_ID(func, bpf_kfunc_call_test_pass_ctx)
> >  BTF_ID(func, bpf_kfunc_call_test_pass1)
> >  BTF_ID(func, bpf_kfunc_call_test_pass2)
> > @@ -682,6 +689,7 @@ BTF_SET_END(test_sk_check_kfunc_ids)
> >
> >  BTF_SET_START(test_sk_acquire_kfunc_ids)
> >  BTF_ID(func, bpf_kfunc_call_test_acquire)
> > +BTF_ID(func, bpf_kfunc_call_test_kptr_get)
> >  BTF_SET_END(test_sk_acquire_kfunc_ids)
> >
> >  BTF_SET_START(test_sk_release_kfunc_ids)
> > @@ -691,8 +699,13 @@ BTF_SET_END(test_sk_release_kfunc_ids)
> >
> >  BTF_SET_START(test_sk_ret_null_kfunc_ids)
> >  BTF_ID(func, bpf_kfunc_call_test_acquire)
> > +BTF_ID(func, bpf_kfunc_call_test_kptr_get)
> >  BTF_SET_END(test_sk_ret_null_kfunc_ids)
> >
> > +BTF_SET_START(test_sk_kptr_acquire_kfunc_ids)
> > +BTF_ID(func, bpf_kfunc_call_test_kptr_get)
> > +BTF_SET_END(test_sk_kptr_acquire_kfunc_ids)
> > +
> >  static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> >  			   u32 size, u32 headroom, u32 tailroom)
> >  {
> > @@ -1579,14 +1592,36 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
> >
> >  static const struct btf_kfunc_id_set bpf_prog_test_kfunc_set = {
> >  	.owner        = THIS_MODULE,
> > -	.check_set    = &test_sk_check_kfunc_ids,
> > -	.acquire_set  = &test_sk_acquire_kfunc_ids,
> > -	.release_set  = &test_sk_release_kfunc_ids,
> > -	.ret_null_set = &test_sk_ret_null_kfunc_ids,
> > +	.check_set        = &test_sk_check_kfunc_ids,
> > +	.acquire_set      = &test_sk_acquire_kfunc_ids,
> > +	.release_set      = &test_sk_release_kfunc_ids,
> > +	.ret_null_set     = &test_sk_ret_null_kfunc_ids,
> > +	.kptr_acquire_set = &test_sk_kptr_acquire_kfunc_ids
> >  };
>
> This hunk probably should have been in the previous patch,
> but since it's not affecting bisect I left it as-is.
>
> > +BTF_ID_LIST(bpf_prog_test_dtor_kfunc_ids)
> > +BTF_ID(struct, prog_test_ref_kfunc)
> > +BTF_ID(func, bpf_kfunc_call_test_release)
> > +BTF_ID(struct, prog_test_member)
> > +BTF_ID(func, bpf_kfunc_call_memb_release)
>
> dtor of prog_test_member doesn't seem to be used ?
>

It is necessary to be able to embed prog_test_member struct as a referenced kptr
in a map value, so it is still called by the map implementation's free path. But
since this is just used for verifier tests related to type matching when
non-zero offset is involved, both acquire and release are dummy functions.

> Please improve dtor and kptr_get test methods for
> struct prog_test_ref_kfunc and prog_test_member to do the real refcnting.
> Empty methods are not testing things fully.

I will leave out prog_test_member since it is not meant to be used at runtime,
but I will add refcount_t to prog_test_ref_kfunc, and include tests for it.

--
Kartikeya