| 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: <CAEf4Bzabc+83abj7gP0h0sCxp-Bajqhm0bdAyn1Gn5bNi5nNXg@mail.gmail.com>
Date: Wed, 22 Jan 2025 09:16:58 -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 Tue, Jan 21, 2025 at 6:43 PM D. Wythe <alibuda@...ux.alibaba.com> wrote:
>
> On Fri, Jan 17, 2025 at 10:36:50AM -0800, Andrii Nakryiko wrote:
> > 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
> > > + 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
> >
>
> Sounds reasonable to me. I will remove the explicit error checks in the
> next version.
>
> > > +
> > > + /* 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
>
> None of them exceed 100 lines. Usually, I would check patches with 85 lines limitations,
> but since 100 lines is acceptable, we can modify it to a single line here for
> better readability.
>
> And thanks very much for your suggestion, I plan to fix these style
> issues in next versions with you ack, is this okay for you?
yep, sgtm
>
> Best wishes,
> D. Wythe
> >
> > > + 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