Thread (11 messages) flat view 11 messages, 2 authors, 2021-02-11

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

From: Andrii Nakryiko <hidden>
Date: 2021-02-10 20:28:05
Also in: bpf

On Tue, Feb 9, 2021 at 12:40 PM Martin KaFai Lau [off-list ref] 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 <redacted>
---
Looks good, see nit below.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
quoted hunk ↗ jump to hunk
 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?
quoted hunk ↗ jump to hunk
@@ -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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help