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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <36cf80a8-a224-4191-b235-50c2b3dd73f6@linux.dev>
Date: Fri, 16 Jan 2026 22:10:12 +0800
From: Leon Hwang <leon.hwang@...ux.dev>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>,
 John Fastabend <john.fastabend@...il.com>,
 Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau
 <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>,
 Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>,
 KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>,
 Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
 Shuah Khan <shuah@...nel.org>, Christian Brauner <brauner@...nel.org>,
 Seth Forshee <sforshee@...nel.org>, Yuichiro Tsuji <yuichtsu@...zon.com>,
 Andrey Albershteyn <aalbersh@...hat.com>,
 Willem de Bruijn <willemb@...gle.com>, Jason Xing
 <kerneljasonxing@...il.com>, Tao Chen <chen.dylane@...ux.dev>,
 Mykyta Yatsenko <yatsenko@...a.com>,
 Kumar Kartikeya Dwivedi <memxor@...il.com>,
 Anton Protopopov <a.s.protopopov@...il.com>, Amery Hung
 <ameryhung@...il.com>, Rong Tao <rongtao@...tc.cn>,
 linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
 linux-kselftest@...r.kernel.org, kernel-patches-bot@...com
Subject: Re: [PATCH bpf-next v5 4/9] bpf: Add syscall common attributes
 support for prog_load



On 2026/1/16 08:54, Andrii Nakryiko wrote:
> On Mon, Jan 12, 2026 at 6:59 AM Leon Hwang <leon.hwang@...ux.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.hwang@...ux.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

[...]