netdev - Re: [PATCH bpf-next v3 03/10] bpf: btf: Introduce helpers for dynamic BTF set registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAADnVQLVpQeejimgdgntYtqw+EN2df1=rcpfxyqygbaHhiddnA@mail.gmail.com>
Date:   Wed, 15 Sep 2021 20:04:34 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v3 03/10] bpf: btf: Introduce helpers for dynamic
 BTF set registration

On Wed, Sep 15, 2021 at 11:06 AM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> On Wed, Sep 15, 2021 at 09:48:35PM IST, Alexei Starovoitov wrote:
> > On Wed, Sep 15, 2021 at 10:39:36AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > This adds helpers for registering btf_id_set from modules and the
> > > check_kfunc_call callback that can be used to look them up.
> > >
> > > With in kernel sets, the way this is supposed to work is, in kernel
> > > callback looks up within the in-kernel kfunc whitelist, and then defers
> > > to the dynamic BTF set lookup if it doesn't find the BTF id. If there is
> > > no in-kernel BTF id set, this callback can be used directly.
> > >
> > > Also fix includes for btf.h and bpfptr.h so that they can included in
> > > isolation. This is in preparation for their usage in tcp_bbr, tcp_cubic
> > > and tcp_dctcp modules in the next patch.
> > >
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> > > ---
> > >  include/linux/bpfptr.h |  1 +
> > >  include/linux/btf.h    | 32 ++++++++++++++++++++++++++
> > >  kernel/bpf/btf.c       | 51 ++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 84 insertions(+)
> > >
> > > diff --git a/include/linux/bpfptr.h b/include/linux/bpfptr.h
> > > index 546e27fc6d46..46e1757d06a3 100644
> > > --- a/include/linux/bpfptr.h
> > > +++ b/include/linux/bpfptr.h
> > > @@ -3,6 +3,7 @@
> > >  #ifndef _LINUX_BPFPTR_H
> > >  #define _LINUX_BPFPTR_H
> > >
> > > +#include <linux/mm.h>
> >
> > Could you explain what this is for?
> >
>
> When e.g. tcp_bbr.c includes btf.h and btf_ids.h without this, it leads to this
> error.
>
>                  from net/ipv4/tcp_bbr.c:59:
> ./include/linux/bpfptr.h: In function ‘kvmemdup_bpfptr’:
> ./include/linux/bpfptr.h:67:19: error: implicit declaration of function ‘kvmalloc’;
>  did you mean ‘kmalloc’? [-Werror=implicit-function-declaration]
>    67 |         void *p = kvmalloc(len, GFP_USER | __GFP_NOWARN);
>       |                   ^~~~~~~~
>       |                   kmalloc
> ./include/linux/bpfptr.h:67:19: warning: initialization of ‘void *’ from ‘int’
>         makes pointer from integer without a cast [-Wint-conversion]
> ./include/linux/bpfptr.h:72:17: error: implicit declaration of function ‘kvfree’;
>         did you mean ‘kfree’? [-Werror=implicit-function-declaration]
>    72 |                 kvfree(p);
>       |                 ^~~~~~
>       |                 kfree

Interesting.
It's because of kvmalloc in kvmemdup_bpfptr.
Which is used in ___bpf_copy_key.
Which is used in map_update_elem.
And afair all maps enforce key_size < KMALLOC_MAX_SIZE.
Not sure why kvmalloc was there.
If it was kmalloc instead then
#include <linux/slab.h>
in linux/sockptr.h that is included by linux/bpfptr.h
would have been enough.
A food for thought.