Re: [PATCH bpf-next v2 1/3] bpf: Allow union argument in trampoline based programs

["Leon Hwang" <leon.hwang@linux.dev>]

On Wed Sep 17, 2025 at 5:35 AM +08, Amery Hung wrote:
> On Tue, Sep 16, 2025 at 8:52 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> Currently, functions with 'union' arguments cannot be traced with
>> fentry/fexit:
>>
>> bpftrace -e 'fentry:release_pages { exit(); }' -v
>> AST node count: 6
>> Attaching 1 probe...
>> ERROR: Error loading BPF program for fentry_vmlinux_release_pages_1.
>> Kernel error log:
>> The function release_pages arg0 type UNION is unsupported.
>> processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
>>
>> ERROR: Loading BPF object(s) failed.
>>
>> The type of the 'release_pages' argument is defined as:
>>
>> typedef union {
>>         struct page **pages;
>>         struct folio **folios;
>>         struct encoded_page **encoded_pages;
>> } release_pages_arg __attribute__ ((__transparent_union__));
>>
>> This patch relaxes the restriction by allowing function arguments of type
>> 'union' to be traced in verifier.
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  include/linux/bpf.h | 3 +++
>>  include/linux/btf.h | 5 +++++
>>  kernel/bpf/btf.c    | 8 +++++---
>>  3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 41f776071ff51..010ecbb798c60 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1119,6 +1119,9 @@ struct bpf_prog_offload {
>>  /* The argument is signed. */
>>  #define BTF_FMODEL_SIGNED_ARG          BIT(1)
>>
>> +/* The argument is a union. */
>> +#define BTF_FMODEL_UNION_ARG           BIT(2)
>> +
>
> [...]
>
>>  struct btf_func_model {
>>         u8 ret_size;
>>         u8 ret_flags;
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index 9eda6b113f9b4..255f8c6bd2438 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -404,6 +404,11 @@ static inline bool btf_type_is_struct(const struct btf_type *t)
>>         return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
>>  }
>>
>> +static inline bool __btf_type_is_union(const struct btf_type *t)
>> +{
>> +       return BTF_INFO_KIND(t->info) == BTF_KIND_UNION;
>> +}
>> +
>>  static inline bool __btf_type_is_struct(const struct btf_type *t)
>>  {
>>         return BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT;
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 64739308902f7..2a85c51412bea 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -6762,7 +6762,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>         /* skip modifiers */
>>         while (btf_type_is_modifier(t))
>>                 t = btf_type_by_id(btf, t->type);
>> -       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>> +       if (btf_type_is_small_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>>                 /* accessing a scalar */
>>                 return true;
>>         if (!btf_type_is_ptr(t)) {
>> @@ -7334,7 +7334,7 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
>>         if (btf_type_is_ptr(t))
>>                 /* kernel size of pointer. Not BPF's size of pointer*/
>>                 return sizeof(void *);
>> -       if (btf_type_is_int(t) || btf_is_any_enum(t) || __btf_type_is_struct(t))
>> +       if (btf_type_is_int(t) || btf_is_any_enum(t) || btf_type_is_struct(t))
>>                 return t->size;
>>         return -EINVAL;
>>  }
>> @@ -7347,6 +7347,8 @@ static u8 __get_type_fmodel_flags(const struct btf_type *t)
>>                 flags |= BTF_FMODEL_STRUCT_ARG;
>
> Might be nit-picking but the handling of union arguments is identical
> to struct, so maybe we don't need to introduce a new flag
> BTF_FMODEL_UNION_ARG just for this. Changing __btf_type_is_struct() to
> btf_type_is_struct() here should also work.

Correct. It should work with such changing.

However, it would be more readable to introduce the new flag as the flag
indicates the argument is a 'union' instead of a 'struct'.

>
> Otherwise, the set looks good to me.
>
> Reviewed-by: Amery Hung <ameryhung@gmail.com>

Thank you for your review.

Thanks,
Leon

[...]