Re: [PATCH bpf-next v5 4/9] bpf: Add syscall common attributes support for prog_load

[Leon Hwang]

On 2026/1/16 08:54, Andrii Nakryiko wrote:
> On Mon, Jan 12, 2026 at 6:59 AM Leon Hwang <leon.hw...@linux.dev> wrote:
>>
>> The log buffer of common attributes would be confusing with the one in
>> 'union bpf_attr' for BPF_PROG_LOAD.
>>
>> In order to clarify the usage of these two log buffers, they both can be
>> used for logging if:
>>
>> * They are same, including 'log_buf', 'log_level' and 'log_size'.
>> * One of them is missing, then another one will be used for logging.
>>
>> If they both have 'log_buf' but they are not same totally, return -EUSERS.
> 
> why use this special error code that we don't seem to use in BPF
> subsystem at all? What's wrong with -EINVAL. This shouldn't be an easy
> mistake to do, tbh.
> 

-EUSERS was suggested by Alexei.

However, I agree with you that it is better to use -EINVAL here.

>>
>> Signed-off-by: Leon Hwang <leon.hw...@linux.dev>
>> ---
>>  include/linux/bpf_verifier.h |  4 +++-
>>  kernel/bpf/log.c             | 29 ++++++++++++++++++++++++++---
>>  kernel/bpf/syscall.c         |  9 ++++++---
>>  3 files changed, 35 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 4c9632c40059..da2d37ca60e7 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -637,9 +637,11 @@ struct bpf_log_attr {
>>         u32 log_level;
>>         struct bpf_attrs *attrs;
>>         u32 offsetof_log_true_size;
>> +       struct bpf_attrs *attrs_common;
>>  };
>>
>> -int bpf_prog_load_log_attr_init(struct bpf_log_attr *log_attr, struct 
>> bpf_attrs *attrs);
>> +int bpf_prog_load_log_attr_init(struct bpf_log_attr *log_attr, struct 
>> bpf_attrs *attrs,
>> +                               struct bpf_attrs *attrs_common);
>>  int bpf_log_attr_finalize(struct bpf_log_attr *log_attr, struct 
>> bpf_verifier_log *log);
>>
>>  #define BPF_MAX_SUBPROGS 256
>> diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
>> index 457b724c4176..eba60a13e244 100644
>> --- a/kernel/bpf/log.c
>> +++ b/kernel/bpf/log.c
>> @@ -865,23 +865,41 @@ void print_insn_state(struct bpf_verifier_env *env, 
>> const struct bpf_verifier_st
>>  }
>>
>>  static int bpf_log_attr_init(struct bpf_log_attr *log_attr, struct 
>> bpf_attrs *attrs, u64 log_buf,
>> -                            u32 log_size, u32 log_level, int 
>> offsetof_log_true_size)
>> +                            u32 log_size, u32 log_level, int 
>> offsetof_log_true_size,
>> +                            struct bpf_attrs *attrs_common)
>>  {
>> +       const struct bpf_common_attr *common_attr = attrs_common ? 
>> attrs_common->attr : NULL;
>> +
> 
> There is something to be said about naming choices here :) it's easy
> to get lost in attrs_common being actually bpf_attrs, which contains
> attr field, which is actually of bpf_common_attr type... It's a bit
> disorienting. :)
> 

I see your point about the naming being confusing.

The original intent of 'struct bpf_attrs' was to provide a shared
wrapper for both 'union bpf_attr' and 'struct bpf_common_attr'. However,
I agree that using 'attrs_common' here makes the layering harder to follow.

If that approach is undesirable, how about introducing a dedicated
structure instead, e.g.:

struct bpf_common_attrs {
        const struct bpf_common_attr *attr;
        bpfptr_t uattr;
        u32 size;
};

This should make the ownership and intent clearer.

Thanks,
Leon

[...]