Re: [PATCH bpf v2 3/3] selftests/bpf: Cover struct_ops bitfield and offset validation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added tests `test_struct_ops_local_bitfield` and `test_struct_ops_bad_member_offset` fail during skeleton load because they forget to disable autocreation of the `testmod_zeroed` map.
--

commit 69d278fe659a33375aee90852a40642cb2fe6e03
Author: Mingpei CAO <caomingpei@gmail.com>

selftests/bpf: Cover struct_ops bitfield and offset validation

This commit adds selftests to verify libbpf's handling of struct_ops bitfield
members and incorrect member offsets. It ensures that unsupported local
bitfields are rejected and bad offsets yield an EINVAL error.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> index 75a0dea511b3f..41029ebb22f01 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
[ ... ]
> @@ -150,6 +151,91 @@ static void test_struct_ops_not_zeroed(void)
>  	struct_ops_module__destroy(skel);
>  }
>  
> +static void test_struct_ops_local_bitfield(void)
> +{
> +	struct struct_ops_module *skel;
> +	char *log = NULL;
> +	int err;
[ ... ]
> +	err = bpf_map__set_autocreate(skel->maps.testmod_local_bitfield, true);
> +	if (!ASSERT_OK(err, "enable_local_bitfield_map"))
> +		goto cleanup;
> +
> +	if (start_libbpf_log_capture())
> +		goto cleanup;
> +	err = struct_ops_module__load(skel);

[Severity: Medium]
Will this skeleton load fail prematurely due to testmod_zeroed?

When struct_ops_module__load() is called, libbpf iterates over all
autocreate maps in the BPF object and initializes them. The testmod_zeroed
map is defined first in the BPF object and is set to autocreate by default.

Since its zeroed_op member is initialized to a non-NULL program pointer but
does not exist in the kernel BTF, bpf_map__init_kern_struct_ops() returns
-ENOTSUP.

This causes the skeleton load to abort early before testmod_local_bitfield
is fully evaluated, leading to false negatives in the test suite. Does
this test need to disable autocreation for the testmod_zeroed map before
calling struct_ops_module__load()?

> +	log = stop_libbpf_log_capture();
> +	if (!ASSERT_EQ(err, -ENOTSUP, "struct_ops_module_load_local_bitfield"))
> +		goto cleanup;
> +	ASSERT_HAS_SUBSTR(log, "local bitfield extra_bitfield is not supported",
> +			  "local_bitfield_rejection_log");
> +
> +cleanup:
> +	free(log);
> +	struct_ops_module__destroy(skel);
> +}
> +
> +static void test_struct_ops_bad_member_offset(void)
> +{
> +	struct struct_ops_module *skel;
> +	struct btf_member *member;
[ ... ]
> +	err = bpf_map__set_autocreate(skel->maps.testmod_bad_offset, true);
> +	if (!ASSERT_OK(err, "enable_bad_offset_map"))
> +		goto cleanup;
> +
> +	if (start_libbpf_log_capture())
> +		goto cleanup;
> +	err = struct_ops_module__load(skel);

[Severity: Medium]
Does this suffer from the same early skeleton load failure?

Since testmod_zeroed is still set to autocreate, the load will return
-ENOTSUP instead of the expected -EINVAL, causing the subsequent ASSERT_EQ
check to fail unconditionally. Should autocreation of testmod_zeroed be
disabled in this function as well?

> +	log = stop_libbpf_log_capture();
> +	if (!ASSERT_EQ(err, -EINVAL, "struct_ops_module_load_bad_offset"))
> +		goto cleanup;
> +	ASSERT_HAS_SUBSTR(log, "member extra is outside the 4-byte local struct_ops type",
> +			  "bad_offset_rejection_log");
> +
> +cleanup:
> +	free(log);
> +	struct_ops_module__destroy(skel);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913145233.1446643-1-caomingpei@gmail.com?part=3