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@xxxxxx> 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@xxxxxx>
> > ---
>
> Looks good, see nit below.
>
> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> > 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.
