diff mbox series

[dwarves,2/3] btf_encoder: emit type tags for bpf_arena pointers

Message ID 20250207021442.155703-3-ihor.solodrai@linux.dev (mailing list archive)
State Superseded
Headers show
Series btf_encoder: emit type tags for bpf_arena pointers | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Ihor Solodrai Feb. 7, 2025, 2:14 a.m. UTC 
When adding a kfunc prototype to BTF, check for the flags indicating
bpf_arena pointers and emit a type tag encoding
__attribute__((address_space(1))) for them. This also requires
updating BTF type ids in the btf_encoder_func_state, which is done as
a side effect in the tagging functions.

This feature depends on recent update in libbpf, supporting arbitrarty
attribute encoding [1].

[1] https://lore.kernel.org/bpf/20250130201239.1429648-1-ihor.solodrai@linux.dev/

Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 btf_encoder.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

Comments

Alan Maguire Feb. 10, 2025, 10:11 p.m. UTC | #1 
On 07/02/2025 02:14, Ihor Solodrai wrote:
> When adding a kfunc prototype to BTF, check for the flags indicating
> bpf_arena pointers and emit a type tag encoding
> __attribute__((address_space(1))) for them. This also requires
> updating BTF type ids in the btf_encoder_func_state, which is done as
> a side effect in the tagging functions.
> 
> This feature depends on recent update in libbpf, supporting arbitrarty
> attribute encoding [1].
> 
> [1] https://lore.kernel.org/bpf/20250130201239.1429648-1-ihor.solodrai@linux.dev/
> 
> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>

a few minor issues below, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  btf_encoder.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index e9f4baf..d7837c2 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -40,7 +40,13 @@
>  #define BTF_SET8_KFUNCS		(1 << 0)
>  #define BTF_KFUNC_TYPE_TAG	"bpf_kfunc"
>  #define BTF_FASTCALL_TAG       "bpf_fastcall"
> -#define KF_FASTCALL            (1 << 12)
> +#define BPF_ARENA_ATTR         "address_space(1)"
> +
> +/* kfunc flags, see include/linux/btf.h in the kernel source */
> +#define KF_FASTCALL   (1 << 12)
> +#define KF_ARENA_RET  (1 << 13)
> +#define KF_ARENA_ARG1 (1 << 14)
> +#define KF_ARENA_ARG2 (1 << 15)
>  
>  struct btf_id_and_flag {
>  	uint32_t id;
> @@ -743,6 +749,91 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
>  	return encoder->type_id_off + tag_type;
>  }
>  
> +static inline struct kfunc_info* btf_encoder__kfunc_by_name(struct btf_encoder *encoder, const char *name) {
> +	struct kfunc_info *kfunc;
> +
> +	list_for_each_entry(kfunc, &encoder->kfuncs, node) {
> +		if (strcmp(kfunc->name, name) == 0)
> +			return kfunc;
> +	}
> +	return NULL;
> +}
> +

above function is only used within #if statement below, right? Should
probably move it there to avoid warnings.

> +#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
> +static int btf_encoder__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
> +{
> +	const struct btf_type *ptr;
> +	int tagged_type_id;
> +
> +	ptr = btf__type_by_id(btf, ptr_id);
> +	if (!btf_is_ptr(ptr))
> +		return -EINVAL;
> +
> +	tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type);
> +	if (tagged_type_id < 0)
> +		return tagged_type_id;
> +
> +	return btf__add_ptr(btf, tagged_type_id);
> +}
> +
> +static int btf_encoder__tag_bpf_arena_arg(struct btf *btf, struct btf_encoder_func_state *state, int idx)
> +{
> +	int id;
> +
> +	if (state->nr_parms <= idx)
> +		return -EINVAL;
> +
> +	id = btf_encoder__tag_bpf_arena_ptr(btf, state->parms[idx].type_id);
> +	if (id < 0) {
> +		btf__log_err(btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, id,
> +			"Error adding BPF_ARENA_ATTR for an argument of kfunc '%s'", state->elf->name);

nit: since we call this for arguments + return value, should we reflect
that in the function name/error message? maybe pass in the KF_ARENA_*
flag or something?

> +		return id;
> +	}
> +	state->parms[idx].type_id = id;
> +
> +	return id;
> +}
> +
> +static int btf_encoder__add_bpf_arena_type_tags(struct btf_encoder *encoder, struct btf_encoder_func_state *state)
> +{
> +	struct kfunc_info *kfunc = NULL;
> +	int ret_type_id;
> +	int err = 0;
> +
> +	if (!state || !state->elf || !state->elf->kfunc)
> +		goto out;
> +
> +	kfunc = btf_encoder__kfunc_by_name(encoder, state->elf->name);
> +	if (!kfunc)
> +		goto out;
> +
> +	if (KF_ARENA_RET & kfunc->flags) {
> +		ret_type_id = btf_encoder__tag_bpf_arena_ptr(encoder->btf, state->ret_type_id);
> +		if (ret_type_id < 0) {
> +			btf__log_err(encoder->btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, ret_type_id,
> +				"Error adding BPF_ARENA_ATTR for return type of kfunc '%s'", state->elf->name);
> +			err = ret_type_id;
> +			goto out;
> +		}
> +		state->ret_type_id = ret_type_id;
> +	}
> +
> +	if (KF_ARENA_ARG1 & kfunc->flags) {
> +		err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 0);
> +		if (err < 0)
> +			goto out;
> +	}
> +
> +	if (KF_ARENA_ARG2 & kfunc->flags) {
> +		err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 1);
> +		if (err < 0)
> +			goto out;
> +	}
> +out:
> +	return err;

not sure we need goto outs here; there are no resources to free etc so
we can just return err/return 0 where appropriate.

> +}
> +#endif // LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
> +
>  static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype,
>  					   struct btf_encoder_func_state *state)
>  {
> @@ -762,6 +853,10 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>  		nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
>  		type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
>  	} else if (state) {
> +#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
> +		if (btf_encoder__add_bpf_arena_type_tags(encoder, state) < 0)

kind of a nit I guess, but I think it might be clearer to make explicit
the work we only have to do for kfuncs, i.e.

		if (state->elf && state->elf->kfunc) {
			/* do kfunc-specific work like arena ptr tag */

		}

I know the function has checks for this internally but I think it makes
it a bit clearer that it's only needed for a small subset of functions,
what do you think?


> +			return -1;
> +#endif
>  		encoder = state->encoder;
>  		btf = state->encoder->btf;
>  		nr_params = state->nr_parms;
Ihor Solodrai Feb. 11, 2025, 12:11 a.m. UTC | #2 
On 2/10/25 2:11 PM, Alan Maguire wrote:
> On 07/02/2025 02:14, Ihor Solodrai wrote:
>> When adding a kfunc prototype to BTF, check for the flags indicating
>> bpf_arena pointers and emit a type tag encoding
>> __attribute__((address_space(1))) for them. This also requires
>> updating BTF type ids in the btf_encoder_func_state, which is done as
>> a side effect in the tagging functions.
>>
>> This feature depends on recent update in libbpf, supporting arbitrarty
>> attribute encoding [1].
>>
>> [1] https://lore.kernel.org/bpf/20250130201239.1429648-1-ihor.solodrai@linux.dev/
>>
>> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>
> a few minor issues below, but
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
>> ---
>>  btf_encoder.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 96 insertions(+), 1 deletion(-)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index e9f4baf..d7837c2 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -40,7 +40,13 @@
>>  #define BTF_SET8_KFUNCS		(1 << 0)
>>  #define BTF_KFUNC_TYPE_TAG	"bpf_kfunc"
>>  #define BTF_FASTCALL_TAG       "bpf_fastcall"
>> -#define KF_FASTCALL            (1 << 12)
>> +#define BPF_ARENA_ATTR         "address_space(1)"
>> +
>> +/* kfunc flags, see include/linux/btf.h in the kernel source */
>> +#define KF_FASTCALL   (1 << 12)
>> +#define KF_ARENA_RET  (1 << 13)
>> +#define KF_ARENA_ARG1 (1 << 14)
>> +#define KF_ARENA_ARG2 (1 << 15)
>>  
>>  struct btf_id_and_flag {
>>  	uint32_t id;
>> @@ -743,6 +749,91 @@ static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
>>  	return encoder->type_id_off + tag_type;
>>  }
>>  
>> +static inline struct kfunc_info* btf_encoder__kfunc_by_name(struct btf_encoder *encoder, const char *name) {
>> +	struct kfunc_info *kfunc;
>> +
>> +	list_for_each_entry(kfunc, &encoder->kfuncs, node) {
>> +		if (strcmp(kfunc->name, name) == 0)
>> +			return kfunc;
>> +	}
>> +	return NULL;
>> +}
>> +
>
> above function is only used within #if statement below, right? Should
> probably move it there to avoid warnings.

Right. I think some of these functions may go away, given Eduard's
suggestions.

>
>> +#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
>> +static int btf_encoder__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
>> +{
>> +	const struct btf_type *ptr;
>> +	int tagged_type_id;
>> +
>> +	ptr = btf__type_by_id(btf, ptr_id);
>> +	if (!btf_is_ptr(ptr))
>> +		return -EINVAL;
>> +
>> +	tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type);
>> +	if (tagged_type_id < 0)
>> +		return tagged_type_id;
>> +
>> +	return btf__add_ptr(btf, tagged_type_id);
>> +}
>> +
>> +static int btf_encoder__tag_bpf_arena_arg(struct btf *btf, struct btf_encoder_func_state *state, int idx)
>> +{
>> +	int id;
>> +
>> +	if (state->nr_parms <= idx)
>> +		return -EINVAL;
>> +
>> +	id = btf_encoder__tag_bpf_arena_ptr(btf, state->parms[idx].type_id);
>> +	if (id < 0) {
>> +		btf__log_err(btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, id,
>> +			"Error adding BPF_ARENA_ATTR for an argument of kfunc '%s'", state->elf->name);
>
> nit: since we call this for arguments + return value, should we reflect
> that in the function name/error message? maybe pass in the KF_ARENA_*
> flag or something?

It is what's happening. btf_encoder__tag_bpf_arena_ptr is called from
btf_encoder__tag_bpf_arena_arg and separately for a return type:

	if (KF_ARENA_RET & kfunc->flags) {
		ret_type_id = btf_encoder__tag_bpf_arena_ptr(encoder->btf, state->ret_type_id);

In both cases the return value is checked and the error message is
different.

I factored out *_arg version of this operation because it has to be
done twice: for _ARG1 and _ARG2.

Maybe I misunderstood you question? lmk

>
>> +		return id;
>> +	}
>> +	state->parms[idx].type_id = id;
>> +
>> +	return id;
>> +}
>> +
>> +static int btf_encoder__add_bpf_arena_type_tags(struct btf_encoder *encoder, struct btf_encoder_func_state *state)
>> +{
>> +	struct kfunc_info *kfunc = NULL;
>> +	int ret_type_id;
>> +	int err = 0;
>> +
>> +	if (!state || !state->elf || !state->elf->kfunc)
>> +		goto out;
>> +
>> +	kfunc = btf_encoder__kfunc_by_name(encoder, state->elf->name);
>> +	if (!kfunc)
>> +		goto out;
>> +
>> +	if (KF_ARENA_RET & kfunc->flags) {
>> +		ret_type_id = btf_encoder__tag_bpf_arena_ptr(encoder->btf, state->ret_type_id);
>> +		if (ret_type_id < 0) {
>> +			btf__log_err(encoder->btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, ret_type_id,
>> +				"Error adding BPF_ARENA_ATTR for return type of kfunc '%s'", state->elf->name);
>> +			err = ret_type_id;
>> +			goto out;
>> +		}
>> +		state->ret_type_id = ret_type_id;
>> +	}
>> +
>> +	if (KF_ARENA_ARG1 & kfunc->flags) {
>> +		err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 0);
>> +		if (err < 0)
>> +			goto out;
>> +	}
>> +
>> +	if (KF_ARENA_ARG2 & kfunc->flags) {
>> +		err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 1);
>> +		if (err < 0)
>> +			goto out;
>> +	}
>> +out:
>> +	return err;
>
> not sure we need goto outs here; there are no resources to free etc so
> we can just return err/return 0 where appropriate.

ack

>
>> +}
>> +#endif // LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
>> +
>>  static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype,
>>  					   struct btf_encoder_func_state *state)
>>  {
>> @@ -762,6 +853,10 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>>  		nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
>>  		type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
>>  	} else if (state) {
>> +#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
>> +		if (btf_encoder__add_bpf_arena_type_tags(encoder, state) < 0)
>
> kind of a nit I guess, but I think it might be clearer to make explicit
> the work we only have to do for kfuncs, i.e.
>
> 		if (state->elf && state->elf->kfunc) {
> 			/* do kfunc-specific work like arena ptr tag */
>
> 		}
>
> I know the function has checks for this internally but I think it makes
> it a bit clearer that it's only needed for a small subset of functions,
> what do you think?

Actually I did write it like that first. IIRC tagging has to be done
before `type_id = state->ret_type_id;` and only when `state` is
passed.

Anyways, I agree it should be clearer that this happens just for some
functions.

>
>
>> +			return -1;
>> +#endif
>>  		encoder = state->encoder;
>>  		btf = state->encoder->btf;
>>  		nr_params = state->nr_parms;
diff mbox series

Patch

diff --git a/btf_encoder.c b/btf_encoder.c
index e9f4baf..d7837c2 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -40,7 +40,13 @@ 
 #define BTF_SET8_KFUNCS		(1 << 0)
 #define BTF_KFUNC_TYPE_TAG	"bpf_kfunc"
 #define BTF_FASTCALL_TAG       "bpf_fastcall"
-#define KF_FASTCALL            (1 << 12)
+#define BPF_ARENA_ATTR         "address_space(1)"
+
+/* kfunc flags, see include/linux/btf.h in the kernel source */
+#define KF_FASTCALL   (1 << 12)
+#define KF_ARENA_RET  (1 << 13)
+#define KF_ARENA_ARG1 (1 << 14)
+#define KF_ARENA_ARG2 (1 << 15)
 
 struct btf_id_and_flag {
 	uint32_t id;
@@ -743,6 +749,91 @@  static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
 	return encoder->type_id_off + tag_type;
 }
 
+static inline struct kfunc_info* btf_encoder__kfunc_by_name(struct btf_encoder *encoder, const char *name) {
+	struct kfunc_info *kfunc;
+
+	list_for_each_entry(kfunc, &encoder->kfuncs, node) {
+		if (strcmp(kfunc->name, name) == 0)
+			return kfunc;
+	}
+	return NULL;
+}
+
+#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
+static int btf_encoder__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
+{
+	const struct btf_type *ptr;
+	int tagged_type_id;
+
+	ptr = btf__type_by_id(btf, ptr_id);
+	if (!btf_is_ptr(ptr))
+		return -EINVAL;
+
+	tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type);
+	if (tagged_type_id < 0)
+		return tagged_type_id;
+
+	return btf__add_ptr(btf, tagged_type_id);
+}
+
+static int btf_encoder__tag_bpf_arena_arg(struct btf *btf, struct btf_encoder_func_state *state, int idx)
+{
+	int id;
+
+	if (state->nr_parms <= idx)
+		return -EINVAL;
+
+	id = btf_encoder__tag_bpf_arena_ptr(btf, state->parms[idx].type_id);
+	if (id < 0) {
+		btf__log_err(btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, id,
+			"Error adding BPF_ARENA_ATTR for an argument of kfunc '%s'", state->elf->name);
+		return id;
+	}
+	state->parms[idx].type_id = id;
+
+	return id;
+}
+
+static int btf_encoder__add_bpf_arena_type_tags(struct btf_encoder *encoder, struct btf_encoder_func_state *state)
+{
+	struct kfunc_info *kfunc = NULL;
+	int ret_type_id;
+	int err = 0;
+
+	if (!state || !state->elf || !state->elf->kfunc)
+		goto out;
+
+	kfunc = btf_encoder__kfunc_by_name(encoder, state->elf->name);
+	if (!kfunc)
+		goto out;
+
+	if (KF_ARENA_RET & kfunc->flags) {
+		ret_type_id = btf_encoder__tag_bpf_arena_ptr(encoder->btf, state->ret_type_id);
+		if (ret_type_id < 0) {
+			btf__log_err(encoder->btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, ret_type_id,
+				"Error adding BPF_ARENA_ATTR for return type of kfunc '%s'", state->elf->name);
+			err = ret_type_id;
+			goto out;
+		}
+		state->ret_type_id = ret_type_id;
+	}
+
+	if (KF_ARENA_ARG1 & kfunc->flags) {
+		err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 0);
+		if (err < 0)
+			goto out;
+	}
+
+	if (KF_ARENA_ARG2 & kfunc->flags) {
+		err = btf_encoder__tag_bpf_arena_arg(encoder->btf, state, 1);
+		if (err < 0)
+			goto out;
+	}
+out:
+	return err;
+}
+#endif // LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
+
 static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype,
 					   struct btf_encoder_func_state *state)
 {
@@ -762,6 +853,10 @@  static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 		nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
 		type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
 	} else if (state) {
+#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
+		if (btf_encoder__add_bpf_arena_type_tags(encoder, state) < 0)
+			return -1;
+#endif
 		encoder = state->encoder;
 		btf = state->encoder->btf;
 		nr_params = state->nr_parms;