netdev - Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 Feb 2021 13:23:17 -0800
From:   Martin KaFai Lau <kafai@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in
 struct_ops

On Wed, Feb 10, 2021 at 12:26:20PM -0800, Andrii Nakryiko wrote:
> On Tue, Feb 9, 2021 at 12:40 PM Martin KaFai Lau <kafai@...com> wrote:
> >
> > When libbpf initializes the kernel's struct_ops in
> > "bpf_map__init_kern_struct_ops()", it enforces all
> > pointer types must be a function pointer and rejects
> > others.  It turns out to be too strict.  For example,
> > when directly using "struct tcp_congestion_ops" from vmlinux.h,
> > it has a "struct module *owner" member and it is set to NULL
> > in a bpf_tcp_cc.o.
> >
> > Instead, it only needs to ensure the member is a function
> > pointer if it has been set (relocated) to a bpf-prog.
> > This patch moves the "btf_is_func_proto(kern_mtype)" check
> > after the existing "if (!prog) { continue; }".
> >
> > The "btf_is_func_proto(mtype)" has already been guaranteed
> > in "bpf_object__collect_st_ops_relos()" which has been run
> > before "bpf_map__init_kern_struct_ops()".  Thus, this check
> > is removed.
> >
> > Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
> > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > ---
> 
> Looks good, see nit below.
> 
> Acked-by: Andrii Nakryiko <andrii@...nel.org>
> 
> >  tools/lib/bpf/libbpf.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 6ae748f6ea11..b483608ea72a 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -887,12 +887,6 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
> >                         kern_mtype = skip_mods_and_typedefs(kern_btf,
> >                                                             kern_mtype->type,
> >                                                             &kern_mtype_id);
> > -                       if (!btf_is_func_proto(mtype) ||
> > -                           !btf_is_func_proto(kern_mtype)) {
> > -                               pr_warn("struct_ops init_kern %s: non func ptr %s is not supported\n",
> > -                                       map->name, mname);
> > -                               return -ENOTSUP;
> > -                       }
> >
> >                         prog = st_ops->progs[i];
> >                         if (!prog) {
> 
> debug message below this line is a bit misleading, it talks about
> "func ptr is not set", but it actually could be any kind of field. So
> it would be nice to just talk about "members" or "fields" there, no?
Good catch.  The debug message needs to change.