[bpf-next,v2,4/5] libbpf: fix error when st-prefix_ops and ops from differ btf

Commit Message

D. Wythe Dec. 10, 2024, 4:04 a.m. UTC

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.

Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
Signed-off-by: D. Wythe <[e-mail]>
---
 tools/lib/bpf/libbpf.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

Comments

Andrii Nakryiko Dec. 12, 2024, 10:24 p.m. UTC | #1

On Mon, Dec 9, 2024 at 8:04 PM D. Wythe <[e-mail]> 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.
>
> Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
> Signed-off-by: D. Wythe <[e-mail]>
> ---
>  tools/lib/bpf/libbpf.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 66173ddb5a2d..046feab4ec36 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -994,6 +994,27 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
>  static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>                                    const char *name, __u32 kind);
>
> +static int
> +find_ksym_btf_id_by_prefix_kind(struct bpf_object *obj, const char *prefix,
> +                               const char *name,
> +                               __u16 kind, struct btf **res_btf,
> +                               struct module_btf **res_mod_btf)
> +{
> +       char btf_type_name[128];
> +       int ret;
> +
> +       ret = snprintf(btf_type_name, sizeof(btf_type_name),
> +                      "%s%s", prefix, name);
> +       /* snprintf returns the number of characters written excluding the
> +        * terminating null. So, if >= BTF_MAX_NAME_SIZE are written, it
> +        * indicates truncation.
> +        */
> +       if (ret < 0 || ret >= sizeof(btf_type_name))
> +               return -ENAMETOOLONG;
> +
> +       return find_ksym_btf_id(obj, btf_type_name, kind, res_btf, res_mod_btf);
> +}

I don't think we need this helper, see below.

> +
>  static int
>  find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
>                            struct module_btf **mod_btf,
> @@ -1028,9 +1049,16 @@ find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
>         kern_vtype_id = find_btf_by_prefix_kind(btf, STRUCT_OPS_VALUE_PREFIX,
>                                                 tname, BTF_KIND_STRUCT);

instead of using find_btf_by_prefix_kind, let's have:

1) snprintf(STRUCT_OPS_VALUE_PREFIX, tname) right here in this
function, so we have expected type constructed and ready to be used
and reused, if necessary
2) call btf__find_by_name_kind() instead of find_btf_by_prefix_kind()
3) if (kern_vtype_id < 0 && !*mod_btf)
      kern_vtype_id = find_ksym_btf_id(...)
4) if (kern_vtype_id < 0) /* now emit error and error out */

>         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;
> +               if (kern_vtype_id == -ENOENT && !*mod_btf)
> +                       kern_vtype_id =
> +                               find_ksym_btf_id_by_prefix_kind(obj, STRUCT_OPS_VALUE_PREFIX,
> +                                                               tname, BTF_KIND_STRUCT, &btf,
> +                                                               mod_btf);
> +               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);
>
> --
> 2.45.0
>

D. Wythe Dec. 16, 2024, 2:14 a.m. UTC | #2

On Thu, Dec 12, 2024 at 02:24:46PM -0800, Andrii Nakryiko wrote:
> On Mon, Dec 9, 2024 at 8:04 PM D. Wythe <[e-mail]> 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 using find_btf_by_prefix_kind, let's have:
> 
> 1) snprintf(STRUCT_OPS_VALUE_PREFIX, tname) right here in this
> function, so we have expected type constructed and ready to be used
> and reused, if necessary
> 2) call btf__find_by_name_kind() instead of find_btf_by_prefix_kind()
> 3) if (kern_vtype_id < 0 && !*mod_btf)
>       kern_vtype_id = find_ksym_btf_id(...)
> 4) if (kern_vtype_id < 0) /* now emit error and error out */

Got it. This looks more concise, I will modify it in the next version in
this way.

Thanks,
D. Wythe

> 
> >         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;
> > +               if (kern_vtype_id == -ENOENT && !*mod_btf)
> > +                       kern_vtype_id =
> > +                               find_ksym_btf_id_by_prefix_kind(obj, STRUCT_OPS_VALUE_PREFIX,
> > +                                                               tname, BTF_KIND_STRUCT, &btf,
> > +                                                               mod_btf);
> > +               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);
> >
> > --
> > 2.45.0
> >

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 66173ddb5a2d..046feab4ec36 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -994,6 +994,27 @@  static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
 static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
 				   const char *name, __u32 kind);
 
+static int
+find_ksym_btf_id_by_prefix_kind(struct bpf_object *obj, const char *prefix,
+				const char *name,
+				__u16 kind, struct btf **res_btf,
+				struct module_btf **res_mod_btf)
+{
+	char btf_type_name[128];
+	int ret;
+
+	ret = snprintf(btf_type_name, sizeof(btf_type_name),
+		       "%s%s", prefix, name);
+	/* snprintf returns the number of characters written excluding the
+	 * terminating null. So, if >= BTF_MAX_NAME_SIZE are written, it
+	 * indicates truncation.
+	 */
+	if (ret < 0 || ret >= sizeof(btf_type_name))
+		return -ENAMETOOLONG;
+
+	return find_ksym_btf_id(obj, btf_type_name, kind, res_btf, res_mod_btf);
+}
+
 static int
 find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
 			   struct module_btf **mod_btf,
@@ -1028,9 +1049,16 @@  find_struct_ops_kern_types(struct bpf_object *obj, const char *tname_raw,
 	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;
+		if (kern_vtype_id == -ENOENT && !*mod_btf)
+			kern_vtype_id =
+				find_ksym_btf_id_by_prefix_kind(obj, STRUCT_OPS_VALUE_PREFIX,
+								tname, BTF_KIND_STRUCT, &btf,
+								mod_btf);
+		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);