netdev - Re: [PATCH bpf-next v3 4/5] libbpf: fix error when st-prefix

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250114071149.GA106114@j66a10360.sqa.eu95>
Date: Tue, 14 Jan 2025 15:11:49 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com    >
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: "D. Wythe" <alibuda@...ux.alibaba.com>, 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 v3 4/5] libbpf: fix error when st-prefix_ops and
 ops from differ btf

On Fri, Jan 10, 2025 at 03:38:19PM -0800, Andrii Nakryiko wrote:
> On Tue, Dec 17, 2024 at 6:44 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_opx_xxx  | xxx         |                                 |
> > +--------+-------------+-------------+---------------------------------+
> > | 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 is searched from the same btf with xxx. In order to
> > handle case 2 correctly without affecting other cases, we cannot simply
> > change the search method for st_ops_xxx from find_btf_by_prefix_kind()
> > to find_ksym_btf_id(), because in this way, case 1 will not be
> > recognized anymore.
> >
> > To address this issue, if st_ops_xxx cannot be found in the btf with xxx
> > and mod_btf does not exist, do find_ksym_btf_id() again to
> > avoid such issue.
> > +               }
> 
> purely from the coding perspective, this is unnecessarily nested and
> convoluted. Wouldn't this work the same but be less nested:
> 
> kern_vtype_id = btf__find_by_name_kind(btf, stname, BTF_KIND_STRUCT);
> if (kern_vtype_id == -ENOENT && !*mod_btf)
>     kern_vtype_id = find_ksym_btf_id(...);
> if (kern_vtype_id < 0) {
>     pr_warn(...);
>     return kern_vtype_id;
> }

Hi Andrii,

It's indeed more concise with your code. Thanks very much for your suggestion.
And Martin has provided us with another suggestion to address this issue,
and according to his plan, there shall be no such complex conditional
checks too.

Anyway, I will keep my code concise in the next version. Thank you for
the reminder.

Best wishes,
D. Wythe

> 
> 
> >         }
> >         kern_vtype = btf__type_by_id(btf, kern_vtype_id);
> >
> > @@ -1046,8 +1055,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
> >