Re: [PATCH 2/2] libbpf: add check if kernel supports kind flag and fix the bitfield members in union and structs if not

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 31/03/2025 21:09, Timur Chernykh wrote:
> Signed-off-by: Timur Chernykh <tim.cherry.co@gmail.com>
> ---
>   tools/lib/bpf/features.c        | 30 ++++++++++++++++
>   tools/lib/bpf/libbpf.c          | 62 ++++++++++++++++++++++++++++++++-
>   tools/lib/bpf/libbpf_internal.h |  2 ++
>   3 files changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
> index 760657f5224c..dfab65f30f0c 100644
> --- a/tools/lib/bpf/features.c
> +++ b/tools/lib/bpf/features.c
> @@ -507,6 +507,33 @@ static int probe_kern_arg_ctx_tag(int token_fd)
>   	return probe_fd(prog_fd);
>   }
>   
> +static int probe_kern_btf_type_kind_flag(int token_fd)
> +{
> +	const char strs[] = "\0bpf_spin_lock\0val\0cnt\0l";
> +	/* struct bpf_spin_lock {
> +	 *   int val;
> +	 * };
> +	 * struct val {
> +	 *   int cnt;
> +	 *   struct bpf_spin_lock l;
> +	 * };
> +	 */
> +	__u32 types[] = {
> +		/* int */
> +		BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4),  /* [1] */
> +		/* struct bpf_spin_lock */                      /* [2] */
> +		BTF_TYPE_ENC(1, BTF_INFO_ENC(BTF_KIND_STRUCT, 1 /* kind bit */, 1), 4),
> +		BTF_MEMBER_ENC(15, 1, 0), /* int val; */
> +		/* struct val */                                /* [3] */
> +		BTF_TYPE_ENC(15, BTF_INFO_ENC(BTF_KIND_STRUCT, 1 /* kind bit */, 2), 8),
> +		BTF_MEMBER_ENC(19, 1, 0), /* int cnt; */
> +		BTF_MEMBER_ENC(23, 2, 32),/* struct bpf_spin_lock l; */
> +	    };
> +
> +	return probe_fd(libbpf__load_raw_btf((char *)types, sizeof(types),
> +			     strs, sizeof(strs), token_fd));
> +}
> +
>   typedef int (*feature_probe_fn)(int /* token_fd */);
>   
>   static struct kern_feature_cache feature_cache;
> @@ -582,6 +609,9 @@ static struct kern_feature_desc {
>   	[FEAT_BTF_QMARK_DATASEC] = {
>   		"BTF DATASEC names starting from '?'", probe_kern_btf_qmark_datasec,
>   	},
> +	[FEAT_BTF_TYPE_KIND_FLAG] = {
> +		"BTF btf_type can have the kind flags set", probe_kern_btf_type_kind_flag,
> +	},
>   };
>   
>   bool feat_supported(struct kern_feature_cache *cache, enum kern_feature_id feat_id)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 8e1edba443dd..392779c10a73 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3113,9 +3113,10 @@ static bool btf_needs_sanitization(struct bpf_object *obj)
>   	bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
>   	bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
>   	bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
> +    bool has_kind_bit_support = kernel_supports(obj, FEAT_BTF_TYPE_KIND_FLAG);
>   
>   	return !has_func || !has_datasec || !has_func_global || !has_float ||
> -	       !has_decl_tag || !has_type_tag || !has_enum64 || !has_qmark_datasec;
> +	       !has_decl_tag || !has_type_tag || !has_enum64 || !has_qmark_datasec || has_kind_bit_support;
>   }
>   
>   static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
> @@ -3128,6 +3129,7 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>   	bool has_type_tag = kernel_supports(obj, FEAT_BTF_TYPE_TAG);
>   	bool has_enum64 = kernel_supports(obj, FEAT_BTF_ENUM64);
>   	bool has_qmark_datasec = kernel_supports(obj, FEAT_BTF_QMARK_DATASEC);
> +	bool has_kind_bit_support = kernel_supports(obj, FEAT_BTF_TYPE_KIND_FLAG);
>   
>   	char name_gen_buff[32] = {0};
>   	int enum64_placeholder_id = 0;
> @@ -3263,6 +3265,64 @@ static int bpf_object__sanitize_btf(struct bpf_object *obj, struct btf *btf)
>   				m->type = enum64_placeholder_id;
>   				m->offset = 0;
>   			}
> +		} else if (!has_kind_bit_support && (btf_is_struct(t) || btf_is_union(t) || btf_is_fwd(t) || btf_is_enum(t) || btf_is_enum64(t))) {
> +			const uint16_t members_cnt = btf_vlen(t);
> +
> +			/* type encoded with a kind flag */
padding seem to be broken here a little bit, I can see some spaces here.
> +		    if (t->info != BTF_INFO_ENC(btf_kind(t), 1, members_cnt)) {
> +		        continue;
> +		    }
> +
> +		    /* unset kind flag anyway */
> +		    t->info = BTF_INFO_ENC(btf_kind(t), 0, btf_vlen(t));
> +
> +		    /* structs an unions has a different bitfield processing behaviour is kind flag is set */
"if kind flag is set" ?
> +		    if (btf_is_struct(t) || btf_is_union(t)) {
btf_is_composite()
> +		        struct btf_member* members = btf_members(t);
> +				struct btf_type* new_int_type = NULL;
> +				int new_int_type_id;
> +				__u32* new_int_type_data;
I'd go with simpler var names:
new_type, new_tid, new_type_data
> +				int encoding = 0;
> +		        int nmember;
padding again. Did you run checkpatch.pl script on this patchset by any 
chance? It should point out these type of style issues?
> +
> +		        for (nmember = 0; nmember < members_cnt; nmember++) {
perhaps i, j or k instead of nmember will do better here.
> +		            struct btf_member* member = &members[nmember];
> +		            const struct btf_type* member_type = btf_type_by_id(btf, member->type);
> +
> +		            while (btf_is_typedef(member_type)) { /* unwrap typedefs */
It looks like you should use `btf__resolve_type` here, as there are not 
only typedefs, but also volatile, const types that you may need to walk
through.
Don't add {} for one line loop or if expression.
> +		                member_type = btf_type_by_id(btf, member_type->type);
> +		            }
> +
> +		            /* bitfields can be only int or enum values */
> +		            if (!(btf_is_int(member_type) || btf_is_enum(member_type))) {
> +		                continue;
> +		            }
> +
> +		            encoding = btf_int_encoding(member_type);
> +		            if (btf_is_enum(member_type) && member_type->info & 0x80000000 /* kind flag */) {
there is `btf_kflag` helper function.
> +		                /* enum value encodes integer signed/unsigned info in the kind flag */
> +		                encoding = BTF_INT_SIGNED;
> +		            }
> +
> +		            /* create new integral type with the same info */
> +		            snprintf(name_gen_buff, sizeof(name_gen_buff), "__int_%d_%d", i, nmember);
> +		            new_int_type_id = btf__add_int(btf, name_gen_buff, member_type->size, encoding);
> +
> +		            if (new_int_type_id < 0) {
> +		                pr_warn("Error adding integer type for a bitfield %d of [%d]", nmember, i);
> +		                return new_int_type_id;
> +		            }
> +
> +		            new_int_type = btf_type_by_id(btf, new_int_type_id);
> +
> +		            /* encode int in legacy way, keep offset 0 and specify bit size as set in the member */
> +		            new_int_type_data = (__u32*)(new_int_type + 1);
> +		            *new_int_type_data = BTF_INT_ENC(encoding, 0, BTF_MEMBER_BITFIELD_SIZE(member->offset));
> +
> +		            member->type = new_int_type_id;
> +		            member->offset = BTF_MEMBER_BIT_OFFSET(member->offset) /* old kernels looks only on offset */;
> +		        }
> +		    }
>   		}
>   	}
>   
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 76669c73dcd1..6369c5520fce 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -380,6 +380,8 @@ enum kern_feature_id {
>   	FEAT_ARG_CTX_TAG,
>   	/* Kernel supports '?' at the front of datasec names */
>   	FEAT_BTF_QMARK_DATASEC,
> +	/* Kernel supports kind flag */
> +	FEAT_BTF_TYPE_KIND_FLAG,
>   	__FEAT_CNT,
>   };
>