Re: [PATCH bpf-next 2/2] selftests/bpf: Add tests for bpf_sockopt is_compat field

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Reading the 4-byte padding at offset 44 in `struct bpf_sockopt` triggers a `verifier_bug` WARN_ONCE because `cg_sockopt_convert_ctx_access()` lacks a translation case.
- [Low] The `getsockopt: read ctx->is_compat` selftest omits the `.io_uring_support = true` flag, reducing test coverage.
--

--- Patch [2]: [PATCH bpf-next 2/2] selftests/bpf: Add tests for bpf_sockopt is_compat field ---
commit 007892bbe60c0ee959b9f1a3e6944cb7ea0af571
Author: Maciej Żenczykowski <[email protected]>

selftests/bpf: Add tests for bpf_sockopt is_compat field

Sync tools/include/uapi/linux/bpf.h with the addition of the is_compat
field in struct bpf_sockopt and add test cases to prog_tests/sockopt.c
to verify context access permissions.

> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index ffd96e8b920bd..15f712b5b1645 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -7616,6 +7616,7 @@ struct bpf_sockopt {
>  	__s32	optname;
>  	__s32	optlen;
>  	__s32	retval;
> +	__s32	is_compat;
>  };

[Severity: High]
Does this addition leave a 4-byte padding at offset 44 that causes a
verifier bug?

Because struct bpf_sockopt contains 64-bit pointers, adding is_compat
makes the fields 44 bytes, expanding the struct size to 48 bytes due to
alignment padding.

In kernel/bpf/cgroup.c, cg_sockopt_is_valid_access() permits accesses up to
the full struct size (48 bytes):

	if (off < 0 || off >= sizeof(struct bpf_sockopt))
		return false;

However, cg_sockopt_convert_ctx_access() lacks a translation case for this
implicit padding at offset 44. If a BPF program reads offset 44, it yields 0
instructions, forcing bpf_convert_ctx_accesses() to emit a verifier_bug
WARN_ONCE.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> index eaac83a7f388b..c7dd22d0e3f84 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> @@ -215,6 +215,49 @@ static struct sockopt_test {
[ ... ]
> +	{
> +		.descr = "getsockopt: read ctx->is_compat",
> +		.insns = {
[ ... ]
> +		},
> +		.attach_type = BPF_CGROUP_GETSOCKOPT,
> +		.expected_attach_type = BPF_CGROUP_GETSOCKOPT,
> +
> +		.get_level = SOL_IP,
> +		.get_optname = IP_TOS,
> +		.get_optlen = 1,
> +	},

[Severity: Low]
Should this test case include the .io_uring_support = true flag?

The "setsockopt: read ctx->is_compat" test configuration further down includes
this flag, but it is omitted here. Adding it would validate io_uring execution
on the new getsockopt hook.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2