[PATCH bpf-next 0/2] Deduplicate type compat check

[PATCH bpf-next 0/2] Deduplicate type compat check

[Daniel Müller]

BPF type compatibility checks (bpf_core_types_are_compat()) are currently
duplicated between kernel and user space. That's a historical artifact more than
intentional doing and can lead to subtle bugs where one implementation is
adjusted but another is forgotten.

That happened with the enum64 work, for example, where the libbpf side was
changed (commit 23b2a3a8f63a ("libbpf: Add enum64 relocation support")) to use
the btf_kind_core_compat() helper function but the kernel side was not (commit
6089fb325cf7 ("bpf: Add btf enum64 support")).

This patch set addresses both the duplication issue and fixes the alluded to
kind check.

For discussion of the topic, please refer to:
https://lore.kernel.org/bpf/CAADnVQKbWR7oarBdewgOBZUPzryhRYvEbkhyPJQHHuxq=0K1gw@mail.gmail.com/T/#mcc99f4a33ad9a322afaf1b9276fb1f0b7add9665

Daniel Müller (2):
  libbpf: Move core "types_are_compat" logic into relo_core.c
  bpf: Use bpf_core_types_are_compat functionality from relo_core.c

 kernel/bpf/btf.c          | 86 +--------------------------------------
 tools/lib/bpf/libbpf.c    | 72 +-------------------------------
 tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++
 tools/lib/bpf/relo_core.h |  2 +
 4 files changed, 83 insertions(+), 155 deletions(-)

-- 
2.30.2

[PATCH bpf-next 1/2] libbpf: Move core "types_are_compat" logic into relo_core.c

[Daniel Müller]

This change merges the two existing implementations of the
bpf_core_types_are_compat() function into relo_core.c, inheriting the
recursion tracking from the kernel and the usage of
btf_kind_core_compat() from libbpf. The kernel is left untouched and
will be adjusted subsequently.

Signed-off-by: Daniel Müller <deso@posteo.net>
---
 tools/lib/bpf/libbpf.c    | 72 +-----------------------------------
 tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++++++
 tools/lib/bpf/relo_core.h |  2 +
 3 files changed, 81 insertions(+), 71 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 49e359c..ca912c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5732,77 +5732,7 @@ bpf_core_find_cands(struct bpf_object *obj, const struct btf *local_btf, __u32 l
 int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
 			      const struct btf *targ_btf, __u32 targ_id)
 {
-	const struct btf_type *local_type, *targ_type;
-	int depth = 32; /* max recursion depth */
-
-	/* caller made sure that names match (ignoring flavor suffix) */
-	local_type = btf__type_by_id(local_btf, local_id);
-	targ_type = btf__type_by_id(targ_btf, targ_id);
-	if (!btf_kind_core_compat(local_type, targ_type))
-		return 0;
-
-recur:
-	depth--;
-	if (depth < 0)
-		return -EINVAL;
-
-	local_type = skip_mods_and_typedefs(local_btf, local_id, &local_id);
-	targ_type = skip_mods_and_typedefs(targ_btf, targ_id, &targ_id);
-	if (!local_type || !targ_type)
-		return -EINVAL;
-
-	if (!btf_kind_core_compat(local_type, targ_type))
-		return 0;
-
-	switch (btf_kind(local_type)) {
-	case BTF_KIND_UNKN:
-	case BTF_KIND_STRUCT:
-	case BTF_KIND_UNION:
-	case BTF_KIND_ENUM:
-	case BTF_KIND_ENUM64:
-	case BTF_KIND_FWD:
-		return 1;
-	case BTF_KIND_INT:
-		/* just reject deprecated bitfield-like integers; all other
-		 * integers are by default compatible between each other
-		 */
-		return btf_int_offset(local_type) == 0 && btf_int_offset(targ_type) == 0;
-	case BTF_KIND_PTR:
-		local_id = local_type->type;
-		targ_id = targ_type->type;
-		goto recur;
-	case BTF_KIND_ARRAY:
-		local_id = btf_array(local_type)->type;
-		targ_id = btf_array(targ_type)->type;
-		goto recur;
-	case BTF_KIND_FUNC_PROTO: {
-		struct btf_param *local_p = btf_params(local_type);
-		struct btf_param *targ_p = btf_params(targ_type);
-		__u16 local_vlen = btf_vlen(local_type);
-		__u16 targ_vlen = btf_vlen(targ_type);
-		int i, err;
-
-		if (local_vlen != targ_vlen)
-			return 0;
-
-		for (i = 0; i < local_vlen; i++, local_p++, targ_p++) {
-			skip_mods_and_typedefs(local_btf, local_p->type, &local_id);
-			skip_mods_and_typedefs(targ_btf, targ_p->type, &targ_id);
-			err = bpf_core_types_are_compat(local_btf, local_id, targ_btf, targ_id);
-			if (err <= 0)
-				return err;
-		}
-
-		/* tail recurse for return type check */
-		skip_mods_and_typedefs(local_btf, local_type->type, &local_id);
-		skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id);
-		goto recur;
-	}
-	default:
-		pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n",
-			btf_kind_str(local_type), local_id, targ_id);
-		return 0;
-	}
+	return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id, INT_MAX);
 }
 
 static size_t bpf_core_hash_fn(const void *key, void *ctx)
diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
index 6ad3c3..e54370 100644
--- a/tools/lib/bpf/relo_core.c
+++ b/tools/lib/bpf/relo_core.c
@@ -141,6 +141,84 @@ static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind)
 	}
 }
 
+int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id,
+				    const struct btf *targ_btf, __u32 targ_id, int level)
+{
+	const struct btf_type *local_type, *targ_type;
+	int depth = 32; /* max recursion depth */
+
+	/* caller made sure that names match (ignoring flavor suffix) */
+	local_type = btf_type_by_id(local_btf, local_id);
+	targ_type = btf_type_by_id(targ_btf, targ_id);
+	if (!btf_kind_core_compat(local_type, targ_type))
+		return 0;
+
+recur:
+	depth--;
+	if (depth < 0)
+		return -EINVAL;
+
+	local_type = skip_mods_and_typedefs(local_btf, local_id, &local_id);
+	targ_type = skip_mods_and_typedefs(targ_btf, targ_id, &targ_id);
+	if (!local_type || !targ_type)
+		return -EINVAL;
+
+	if (!btf_kind_core_compat(local_type, targ_type))
+		return 0;
+
+	switch (btf_kind(local_type)) {
+	case BTF_KIND_UNKN:
+	case BTF_KIND_STRUCT:
+	case BTF_KIND_UNION:
+	case BTF_KIND_ENUM:
+	case BTF_KIND_FWD:
+	case BTF_KIND_ENUM64:
+		return 1;
+	case BTF_KIND_INT:
+		/* just reject deprecated bitfield-like integers; all other
+		 * integers are by default compatible between each other
+		 */
+		return btf_int_offset(local_type) == 0 && btf_int_offset(targ_type) == 0;
+	case BTF_KIND_PTR:
+		local_id = local_type->type;
+		targ_id = targ_type->type;
+		goto recur;
+	case BTF_KIND_ARRAY:
+		local_id = btf_array(local_type)->type;
+		targ_id = btf_array(targ_type)->type;
+		goto recur;
+	case BTF_KIND_FUNC_PROTO: {
+		struct btf_param *local_p = btf_params(local_type);
+		struct btf_param *targ_p = btf_params(targ_type);
+		__u16 local_vlen = btf_vlen(local_type);
+		__u16 targ_vlen = btf_vlen(targ_type);
+		int i, err;
+
+		if (local_vlen != targ_vlen)
+			return 0;
+
+		for (i = 0; i < local_vlen; i++, local_p++, targ_p++) {
+			if (level <= 0)
+				return -EINVAL;
+
+			skip_mods_and_typedefs(local_btf, local_p->type, &local_id);
+			skip_mods_and_typedefs(targ_btf, targ_p->type, &targ_id);
+			err = bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf,
+							      targ_id, level - 1);
+			if (err <= 0)
+				return err;
+		}
+
+		/* tail recurse for return type check */
+		skip_mods_and_typedefs(local_btf, local_type->type, &local_id);
+		skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id);
+		goto recur;
+	}
+	default:
+		return 0;
+	}
+}
+
 /*
  * Turn bpf_core_relo into a low- and high-level spec representation,
  * validating correctness along the way, as well as calculating resulting
diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
index 7df0da0..b8998f 100644
--- a/tools/lib/bpf/relo_core.h
+++ b/tools/lib/bpf/relo_core.h
@@ -68,6 +68,8 @@ struct bpf_core_relo_res {
 	__u32 new_type_id;
 };
 
+int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id,
+				    const struct btf *targ_btf, __u32 targ_id, int level);
 int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
 			      const struct btf *targ_btf, __u32 targ_id);
 
-- 
2.30.2

Re: [PATCH bpf-next 1/2] libbpf: Move core "types_are_compat" logic into relo_core.c

[Daniel Müller]

On Wed, Jun 22, 2022 at 05:35:05PM +0000, Daniel Müller wrote:
> diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> index 6ad3c3..e54370 100644
> --- a/tools/lib/bpf/relo_core.c
> +++ b/tools/lib/bpf/relo_core.c
> @@ -141,6 +141,84 @@ static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind)
>  	}
>  }
>  
> +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id,
> +				    const struct btf *targ_btf, __u32 targ_id, int level)
> +{

[...]

> +
> +		/* tail recurse for return type check */
> +		skip_mods_and_typedefs(local_btf, local_type->type, &local_id);
> +		skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id);
> +		goto recur;
> +	}
> +	default:
> +		return 0;

I actually left out the pr_warn here (present in libbpf but not the kernel). I
suppose it would be best to carry it over from libbpf?

Thanks,
Daniel

Re: [PATCH bpf-next 1/2] libbpf: Move core "types_are_compat" logic into relo_core.c

[Andrii Nakryiko]

On Wed, Jun 22, 2022 at 3:19 PM Daniel Müller <deso@posteo.net> wrote:
>
> On Wed, Jun 22, 2022 at 05:35:05PM +0000, Daniel Müller wrote:
> > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > index 6ad3c3..e54370 100644
> > --- a/tools/lib/bpf/relo_core.c
> > +++ b/tools/lib/bpf/relo_core.c
> > @@ -141,6 +141,84 @@ static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind)
> >       }
> >  }
> >
> > +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id,
> > +                                 const struct btf *targ_btf, __u32 targ_id, int level)
> > +{
>
> [...]
>
> > +
> > +             /* tail recurse for return type check */
> > +             skip_mods_and_typedefs(local_btf, local_type->type, &local_id);
> > +             skip_mods_and_typedefs(targ_btf, targ_type->type, &targ_id);
> > +             goto recur;
> > +     }
> > +     default:
> > +             return 0;
>
> I actually left out the pr_warn here (present in libbpf but not the kernel). I
> suppose it would be best to carry it over from libbpf?

Yes, please.


>
> Thanks,
> Daniel

Re: [PATCH bpf-next 1/2] libbpf: Move core "types_are_compat" logic into relo_core.c

[Andrii Nakryiko]

On Wed, Jun 22, 2022 at 10:35 AM Daniel Müller <deso@posteo.net> wrote:
>
> This change merges the two existing implementations of the
> bpf_core_types_are_compat() function into relo_core.c, inheriting the
> recursion tracking from the kernel and the usage of
> btf_kind_core_compat() from libbpf. The kernel is left untouched and
> will be adjusted subsequently.
>
> Signed-off-by: Daniel Müller <deso@posteo.net>
> ---

I don't feel very strongly about this, but given we are consolidating
kernel and libbpf code, I think it makes sense to do it in one patch.

>  tools/lib/bpf/libbpf.c    | 72 +-----------------------------------
>  tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++++++
>  tools/lib/bpf/relo_core.h |  2 +
>  3 files changed, 81 insertions(+), 71 deletions(-)
>

[...]

> -       default:
> -               pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n",
> -                       btf_kind_str(local_type), local_id, targ_id);
> -               return 0;
> -       }
> +       return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id, INT_MAX);

INT_MAX seems like an overkill, let's just hard-code 32 just like we
have for a local recursion limit here?

>  }
>

[...]

>  /*
>   * Turn bpf_core_relo into a low- and high-level spec representation,
>   * validating correctness along the way, as well as calculating resulting
> diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
> index 7df0da0..b8998f 100644
> --- a/tools/lib/bpf/relo_core.h
> +++ b/tools/lib/bpf/relo_core.h
> @@ -68,6 +68,8 @@ struct bpf_core_relo_res {
>         __u32 new_type_id;
>  };
>
> +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id,
> +                                   const struct btf *targ_btf, __u32 targ_id, int level);

Just leave it called __bpf_core_types_are_compat like in kernel, it is
clearly an "internal" version of bpf_core_types_are_compat(), so it's
more proper naming convention.


>  int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
>                               const struct btf *targ_btf, __u32 targ_id);
>
> --
> 2.30.2
>

Re: [PATCH bpf-next 1/2] libbpf: Move core "types_are_compat" logic into relo_core.c

[Daniel Müller]

On Wed, Jun 22, 2022 at 09:15:01PM -0700, Andrii Nakryiko wrote:
> On Wed, Jun 22, 2022 at 10:35 AM Daniel Müller <deso@posteo.net> wrote:
> >
> > This change merges the two existing implementations of the
> > bpf_core_types_are_compat() function into relo_core.c, inheriting the
> > recursion tracking from the kernel and the usage of
> > btf_kind_core_compat() from libbpf. The kernel is left untouched and
> > will be adjusted subsequently.
> >
> > Signed-off-by: Daniel Müller <deso@posteo.net>
> > ---
> 
> I don't feel very strongly about this, but given we are consolidating
> kernel and libbpf code, I think it makes sense to do it in one patch.

Sure.


> >  tools/lib/bpf/libbpf.c    | 72 +-----------------------------------
> >  tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/relo_core.h |  2 +
> >  3 files changed, 81 insertions(+), 71 deletions(-)
> >
> 
> [...]
> 
> > -       default:
> > -               pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n",
> > -                       btf_kind_str(local_type), local_id, targ_id);
> > -               return 0;
> > -       }
> > +       return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id, INT_MAX);
> 
> INT_MAX seems like an overkill, let's just hard-code 32 just like we
> have for a local recursion limit here?

Okay.

> >  }
> >
> 
> [...]
> 
> >  /*
> >   * Turn bpf_core_relo into a low- and high-level spec representation,
> >   * validating correctness along the way, as well as calculating resulting
> > diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
> > index 7df0da0..b8998f 100644
> > --- a/tools/lib/bpf/relo_core.h
> > +++ b/tools/lib/bpf/relo_core.h
> > @@ -68,6 +68,8 @@ struct bpf_core_relo_res {
> >         __u32 new_type_id;
> >  };
> >
> > +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id,
> > +                                   const struct btf *targ_btf, __u32 targ_id, int level);
> 
> Just leave it called __bpf_core_types_are_compat like in kernel, it is
> clearly an "internal" version of bpf_core_types_are_compat(), so it's
> more proper naming convention.

Sounds good.

[...]

Thanks,
Daniel

[PATCH bpf-next 2/2] bpf: Use bpf_core_types_are_compat functionality from relo_core.c

[Daniel Müller]

This change adjusts the kernel BPF code to use
bpf_core_types_are_compat_recur() from relo_core.c for its
implementation of bpf_core_types_are_compat(). In so doing we fix an
oversight where we were still comparing local and target BTF kinds for
equality directly, as opposed to using btf_kind_core_compat() which
correctly handles enum/enum64 kinds.

Signed-off-by: Daniel Müller <deso@posteo.net>
---
 kernel/bpf/btf.c | 86 ++----------------------------------------------
 1 file changed, 2 insertions(+), 84 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f08037..031511b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7416,87 +7416,6 @@ EXPORT_SYMBOL_GPL(register_btf_id_dtor_kfuncs);
 
 #define MAX_TYPES_ARE_COMPAT_DEPTH 2
 
-static
-int __bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
-				const struct btf *targ_btf, __u32 targ_id,
-				int level)
-{
-	const struct btf_type *local_type, *targ_type;
-	int depth = 32; /* max recursion depth */
-
-	/* caller made sure that names match (ignoring flavor suffix) */
-	local_type = btf_type_by_id(local_btf, local_id);
-	targ_type = btf_type_by_id(targ_btf, targ_id);
-	if (btf_kind(local_type) != btf_kind(targ_type))
-		return 0;
-
-recur:
-	depth--;
-	if (depth < 0)
-		return -EINVAL;
-
-	local_type = btf_type_skip_modifiers(local_btf, local_id, &local_id);
-	targ_type = btf_type_skip_modifiers(targ_btf, targ_id, &targ_id);
-	if (!local_type || !targ_type)
-		return -EINVAL;
-
-	if (btf_kind(local_type) != btf_kind(targ_type))
-		return 0;
-
-	switch (btf_kind(local_type)) {
-	case BTF_KIND_UNKN:
-	case BTF_KIND_STRUCT:
-	case BTF_KIND_UNION:
-	case BTF_KIND_ENUM:
-	case BTF_KIND_FWD:
-	case BTF_KIND_ENUM64:
-		return 1;
-	case BTF_KIND_INT:
-		/* just reject deprecated bitfield-like integers; all other
-		 * integers are by default compatible between each other
-		 */
-		return btf_int_offset(local_type) == 0 && btf_int_offset(targ_type) == 0;
-	case BTF_KIND_PTR:
-		local_id = local_type->type;
-		targ_id = targ_type->type;
-		goto recur;
-	case BTF_KIND_ARRAY:
-		local_id = btf_array(local_type)->type;
-		targ_id = btf_array(targ_type)->type;
-		goto recur;
-	case BTF_KIND_FUNC_PROTO: {
-		struct btf_param *local_p = btf_params(local_type);
-		struct btf_param *targ_p = btf_params(targ_type);
-		__u16 local_vlen = btf_vlen(local_type);
-		__u16 targ_vlen = btf_vlen(targ_type);
-		int i, err;
-
-		if (local_vlen != targ_vlen)
-			return 0;
-
-		for (i = 0; i < local_vlen; i++, local_p++, targ_p++) {
-			if (level <= 0)
-				return -EINVAL;
-
-			btf_type_skip_modifiers(local_btf, local_p->type, &local_id);
-			btf_type_skip_modifiers(targ_btf, targ_p->type, &targ_id);
-			err = __bpf_core_types_are_compat(local_btf, local_id,
-							  targ_btf, targ_id,
-							  level - 1);
-			if (err <= 0)
-				return err;
-		}
-
-		/* tail recurse for return type check */
-		btf_type_skip_modifiers(local_btf, local_type->type, &local_id);
-		btf_type_skip_modifiers(targ_btf, targ_type->type, &targ_id);
-		goto recur;
-	}
-	default:
-		return 0;
-	}
-}
-
 /* Check local and target types for compatibility. This check is used for
  * type-based CO-RE relocations and follow slightly different rules than
  * field-based relocations. This function assumes that root types were already
@@ -7519,9 +7438,8 @@ int __bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
 int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
 			      const struct btf *targ_btf, __u32 targ_id)
 {
-	return __bpf_core_types_are_compat(local_btf, local_id,
-					   targ_btf, targ_id,
-					   MAX_TYPES_ARE_COMPAT_DEPTH);
+	return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id,
+					       MAX_TYPES_ARE_COMPAT_DEPTH);
 }
 
 static bool bpf_core_is_flavor_sep(const char *s)
-- 
2.30.2