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?
> @@ -901,6 +895,12 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
> continue;
> }
>
> + if (!btf_is_func_proto(kern_mtype)) {
> + pr_warn("struct_ops init_kern %s: kernel member %s is not a func ptr\n",
> + map->name, mname);
> + return -ENOTSUP;
> + }
> +
> prog->attach_btf_id = kern_type_id;
> prog->expected_attach_type = kern_member_idx;
>
> --
> 2.24.1
>
