On Fri, Jan 16, 2026 at 6:10 AM Leon Hwang <[email protected]> wrote: > > > > On 2026/1/16 08:54, Andrii Nakryiko wrote: > > On Mon, Jan 12, 2026 at 6:59 AM Leon Hwang <[email protected]> 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.
I don't know what the context was, if you can find it that would be great. Maybe special error makes sense for what Alexei had in mind. > > >> > >> Signed-off-by: Leon Hwang <[email protected]> > >> --- > >> 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. I don't know and it's not that important, as it's pretty content. But I'd just try to shorten some names, maybe just "common" for internal helpers would make sense. common->log_buf, seems to work. > > Thanks, > Leon > > [...] >
