Re: [PATCH bpf-next v14 1/8] bpf: Extend BPF syscall with common attributes support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Implicit compiler padding in UAPI `struct bpf_common_attr` compromises backward compatibility.
- [High] `BPF_COMMON_ATTRS` flag is silently accepted and ignored for unsupported BPF commands, breaking UAPI feature probing.
--

commit 8985d7da06248bc1e83a6cb46060187b0440e549
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Extend BPF syscall with common attributes support

This commit adds generic BPF syscall support for passing common attributes
like log_buf, log_size, log_level, and log_true_size. It introduces a new
UAPI struct bpf_common_attr and a BPF_COMMON_ATTRS command flag bit to
indicate when these attributes are provided.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h

[ ... ]

> @@ -1500,6 +1501,13 @@ struct bpf_stack_build_id {
>  	};
>  };
>  
> +struct bpf_common_attr {
> +	__aligned_u64 log_buf;
> +	__u32 log_size;
> +	__u32 log_level;
> +	__u32 log_true_size;
> +};

Does this UAPI structure contain implicit trailing padding? With one
__aligned_u64 (8 bytes) and three __u32 fields (12 bytes), there are 20
bytes of data. Because of the 8-byte alignment, the compiler will pad the
structure to 24 bytes.

If a future kernel extension adds a new 4-byte field, older userspace programs
allocating this structure on the stack might inadvertently pass uninitialized
stack garbage into the new field, permanently breaking backward compatibility.

Should this include an explicit __u32 pad; field, and should the kernel
explicitly validate that attr_common.pad == 0?

[ ... ]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c

[ ... ]

> @@ -6226,6 +6228,20 @@ static int __sys_bpf(enum bpf_cmd cmd, bpfptr_t uattr, unsigned int size)
>  	if (copy_from_bpfptr(&attr, uattr, size) != 0)
>  		return -EFAULT;
>  
> +	memset(&attr_common, 0, sizeof(attr_common));
> +	if (cmd & BPF_COMMON_ATTRS) {
> +		err = bpf_check_uarg_tail_zero(uattr_common, sizeof(attr_common), size_common);
> +		if (err)
> +			return err;
> +
> +		cmd &= ~BPF_COMMON_ATTRS;

By unconditionally stripping BPF_COMMON_ATTRS from the command here, won't any
BPF command (such as BPF_MAP_FREEZE) successfully accept the flag but silently
ignore the provided common attributes?

This might prevent userspace from correctly probing whether the running kernel
actually supports common attributes for a given command.

Should __sys_bpf() enforce a whitelist of supported commands before stripping
the flag, or explicitly reject the flag for commands that do not support it?

> +		size_common = min_t(u32, size_common, sizeof(attr_common));
> +		if (copy_from_bpfptr(&attr_common, uattr_common, size_common) != 0)
> +			return -EFAULT;
> +	} else {
> +		size_common = 0;
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512153157.28382-1-leon.hwang@linux.dev?part=1