| 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
| ||
|
Message-ID: <CAEf4BzZvxqiQ2J1XQMm-ZDBjSsmtJJk6-_RbexPk9vWxAO=ksw@mail.gmail.com>
Date: Fri, 17 Jan 2025 10:36:50 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: "D. Wythe" <alibuda@...ux.alibaba.com>
Cc: kgraul@...ux.ibm.com, wenjia@...ux.ibm.com, jaka@...ux.ibm.com,
ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev,
pabeni@...hat.com, song@...nel.org, sdf@...gle.com, haoluo@...gle.com,
yhs@...com, edumazet@...gle.com, john.fastabend@...il.com, kpsingh@...nel.org,
jolsa@...nel.org, guwen@...ux.alibaba.com, kuba@...nel.org,
davem@...emloft.net, netdev@...r.kernel.org, linux-s390@...r.kernel.org,
linux-rdma@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v6 4/5] libbpf: fix error when st-prefix_ops and
ops from differ btf
On Wed, Jan 15, 2025 at 11:45 PM D. Wythe <alibuda@...ux.alibaba.com> wrote:
>
> When a struct_ops named xxx_ops was registered by a module, and
> it will be used in both built-in modules and the module itself,
> so that the btf_type of xxx_ops will be present in btf_vmlinux
> instead of in btf_mod, which means that the btf_type of
> bpf_struct_ops_xxx_ops and xxx_ops will not be in the same btf.
>
> Here are four possible case:
>
> +--------+---------------+-------------+---------------------------------+
> | | st_ops_xxx_ops| xxx_ops | |
> +--------+---------------+-------------+---------------------------------+
> | case 0 | btf_vmlinux | bft_vmlinux | be used and reg only in vmlinux |
> +--------+---------------+-------------+---------------------------------+
> | case 1 | btf_vmlinux | bpf_mod | INVALID |
> +--------+---------------+-------------+---------------------------------+
> | case 2 | btf_mod | btf_vmlinux | reg in mod but be used both in |
> | | | | vmlinux and mod. |
> +--------+---------------+-------------+---------------------------------+
> | case 3 | btf_mod | btf_mod | be used and reg only in mod |
> +--------+---------------+-------------+---------------------------------+
>
> At present, cases 0, 1, and 3 can be correctly identified, because
> st_ops_xxx_ops is searched from the same btf with xxx_ops. In order to
> handle case 2 correctly without affecting other cases, we cannot simply
> change the search method for st_ops_xxx_ops from find_btf_by_prefix_kind()
> to find_ksym_btf_id(), because in this way, case 1 will not be
> recognized anymore.
>
> To address the issue, we always look for st_ops_xxx_ops first,
> figure out the btf, and then look for xxx_ops with the very btf to avoid
> such issue.
>
> Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
> Signed-off-by: D. Wythe <alibuda@...ux.alibaba.com>
> ---
> tools/lib/bpf/libbpf.c | 41 +++++++++++++++++++++++------------------
> 1 file changed, 23 insertions(+), 18 deletions(-)
>
Other than a few nits below, LGTM
Acked-by: Andrii Nakryiko <andrii@...nel.org>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 66173ddb5a2d..202bc4c1001e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1005,14 +1005,33 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
> const struct btf_member *kern_data_member;
> struct btf *btf = NULL;
> __s32 kern_vtype_id, kern_type_id;
> - char tname[256];
> + char tname[256], stname[256];
> + int ret;
> __u32 i;
>
> snprintf(tname, sizeof(tname), "%.*s",
> (int)bpf_core_essential_name_len(tname_raw), tname_raw);
>
> - kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
> + ret = snprintf(stname, sizeof(stname), "%s%s", STRUCT_OPS_VALUE_PREFIX,
> + tname);
> + if (ret < 0 || ret >= sizeof(stname))
> + return -ENAMETOOLONG;
see preexisting snprintf() above, we don't really handle truncation
errors explicitly, they are extremely unlikely and not expected at
all, and worst case nothing will be found and user will get some
-ENOENT or something like that eventually. I'd drop this extra error
checking and keep it streamlines, similar to tname
> +
> + /* Look for the corresponding "map_value" type that will be used
> + * in map_update(BPF_MAP_TYPE_STRUCT_OPS) first, figure out the btf
> + * and the mod_btf.
> + * For example, find "struct bpf_struct_ops_tcp_congestion_ops".
> + */
> + kern_vtype_id = find_ksym_btf_id(obj, stname, BTF_KIND_STRUCT,
> &btf, mod_btf);
nit: if this fits under 100 characters, keep it single line
> + if (kern_vtype_id < 0) {
> + pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
> + stname);
same nit about preserving single-line statements as much as possible,
they are much easier to read
> + return kern_vtype_id;
> + }
> + kern_vtype = btf__type_by_id(btf, kern_vtype_id);
> +
> + kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);
> if (kern_type_id < 0) {
> pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
> tname);
> @@ -1020,20 +1039,6 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
> }
> kern_type = btf__type_by_id(btf, kern_type_id);
>
> - /* Find the corresponding "map_value" type that will be used
> - * in map_update(BPF_MAP_TYPE_STRUCT_OPS). For example,
> - * find "struct bpf_struct_ops_tcp_congestion_ops" from the
> - * btf_vmlinux.
> - */
> - kern_vtype_id = find_btf_by_prefix_kind(btf, STRUCT_OPS_VALUE_PREFIX,
> - tname, BTF_KIND_STRUCT);
> - if (kern_vtype_id < 0) {
> - pr_warn("struct_ops init_kern: struct %s%s is not found in kernel BTF\n",
> - STRUCT_OPS_VALUE_PREFIX, tname);
> - return kern_vtype_id;
> - }
> - kern_vtype = btf__type_by_id(btf, kern_vtype_id);
> -
> /* Find "struct tcp_congestion_ops" from
> * struct bpf_struct_ops_tcp_congestion_ops {
> * [ ... ]
> @@ -1046,8 +1051,8 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
> break;
> }
> if (i == btf_vlen(kern_vtype)) {
> - pr_warn("struct_ops init_kern: struct %s data is not found in struct %s%s\n",
> - tname, STRUCT_OPS_VALUE_PREFIX, tname);
> + pr_warn("struct_ops init_kern: struct %s data is not found in struct %s\n",
> + tname, stname);
> return -EINVAL;
> }
>
> --
> 2.45.0
>
Powered by blists - more mailing lists