Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220712145850.599666-1-benjamin.tissoires@redhat.com>
 <20220712145850.599666-7-benjamin.tissoires@redhat.com> <bf56b01d-4c05-0d0b-e85b-219e55606803@fb.com>
In-Reply-To: <bf56b01d-4c05-0d0b-e85b-219e55606803@fb.com>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Mon, 18 Jul 2022 10:42:13 +0200
Message-ID: <CAO-hwJJirS9S8TU9NMXhjmaTeL9PNxeQBg0oT3zLdd63uDp74g@mail.gmail.com>
Subject: Re: [PATCH bpf-next v6 06/23] selftests/bpf: Add tests for kfunc
 returning a memory pointer
To:     Yonghong Song <yhs@fb.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Jiri Kosina <jikos@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, Shuah Khan <shuah@kernel.org>,
        Dave Marchevsky <davemarchevsky@fb.com>,
        Joe Stringer <joe@cilium.io>, Jonathan Corbet <corbet@lwn.net>,
        Tero Kristo <tero.kristo@linux.intel.com>,
        lkml <linux-kernel@vger.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Linux Doc Mailing List <linux-doc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Jul 16, 2022 at 6:34 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/12/22 7:58 AM, Benjamin Tissoires wrote:
> > We add 2 new kfuncs that are following the RET_PTR_TO_MEM
> > capability from the previous commit.
> > Then we test them in selftests:
> > the first tests are testing valid case, and are not failing,
> > and the later ones are actually preventing the program to be loaded
> > because they are wrong.
> >
> > To work around that, we mark the failing ones as not autoloaded
> > (with SEC("?tc")), and we manually enable them one by one, ensuring
> > the verifier rejects them.
> >
> > To be able to use bpf_program__set_autoload() from libbpf, we need
> > to use a plain skeleton, not a light-skeleton, and this is why we
> > also change the Makefile to generate both for kfunc_call_test.c
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > ---
> >
> > new in v6
> > ---
> >   include/linux/btf.h                           |  4 +-
> >   net/bpf/test_run.c                            | 22 +++++
> >   tools/testing/selftests/bpf/Makefile          |  5 +-
> >   .../selftests/bpf/prog_tests/kfunc_call.c     | 48 ++++++++++
> >   .../selftests/bpf/progs/kfunc_call_test.c     | 89 +++++++++++++++++++
> >   5 files changed, 165 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 31da4273c2ec..6f46ff2128ae 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -422,7 +422,9 @@ static inline int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dt
> >
> >   static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type *t)
> >   {
> > -     /* t comes in already as a pointer */
> > +     if (!btf_type_is_ptr(t))
> > +             return false;
>
> Why we have a change here?

Definitely a mistake while fixing/rebasing the series.

Will bring this hunk in the previous patch in the next revision.

Thanks for the review!

Cheers,
Benjamin

>
> > +
> >       t = btf_type_by_id(btf, t->type);
> >
> >       /* allow const */
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index 9da2a42811e8..0b4026ea4652 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -606,6 +606,24 @@ noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
> >       WARN_ON_ONCE(1);
> >   }
> >
> > +static int *__bpf_kfunc_call_test_get_mem(struct prog_test_ref_kfunc *p, const int size)
> > +{
> > +     if (size > 2 * sizeof(int))
> > +             return NULL;
> > +
> > +     return (int *)p;
> > +}
> > +
> > +noinline int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size)
> > +{
> > +     return __bpf_kfunc_call_test_get_mem(p, rdwr_buf_size);
> > +}
> > +
> > +noinline int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size)
> > +{
> > +     return __bpf_kfunc_call_test_get_mem(p, rdonly_buf_size);
> > +}
> > +
> >   noinline struct prog_test_ref_kfunc *
> >   bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **pp, int a, int b)
> >   {
> > @@ -704,6 +722,8 @@ BTF_ID(func, bpf_kfunc_call_memb_acquire)
> >   BTF_ID(func, bpf_kfunc_call_test_release)
> >   BTF_ID(func, bpf_kfunc_call_memb_release)
> >   BTF_ID(func, bpf_kfunc_call_memb1_release)
> > +BTF_ID(func, bpf_kfunc_call_test_get_rdwr_mem)
> > +BTF_ID(func, bpf_kfunc_call_test_get_rdonly_mem)
> >   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)
> > @@ -731,6 +751,8 @@ 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_memb_acquire)
> > +BTF_ID(func, bpf_kfunc_call_test_get_rdwr_mem)
> > +BTF_ID(func, bpf_kfunc_call_test_get_rdonly_mem)
> >   BTF_ID(func, bpf_kfunc_call_test_kptr_get)
> >   BTF_SET_END(test_sk_ret_null_kfunc_ids)
> >
> [...]
>