| Message ID | 20210209193105.1752743-1-kafai@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,1/2] libbpf: Ignore non function pointer member in struct_ops | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: yhs@fb.com songliubraving@fb.com kpsingh@kernel.org john.fastabend@gmail.com andrii@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Feb 9, 2021 at 12:40 PM Martin KaFai Lau <kafai@fb.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@fb.com> > --- Looks good, see nit below. Acked-by: Andrii Nakryiko <andrii@kernel.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? > @@ -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 >
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@fb.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@fb.com> > > --- > > Looks good, see nit below. > > Acked-by: Andrii Nakryiko <andrii@kernel.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.
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) { @@ -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;
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@fb.com> --- tools/lib/bpf/libbpf.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)