Re: [PATCH v2 bpf-next 18/18] selftests/bpf: Add a test verifying inline information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Parser desynchronization in module_get_base_addr() when reading trailing fields in /proc/modules.
- [Medium] Inverted logic in ASSERT_LT check causes early exit on test success.
- [Medium] Unhandled error from btf__parse_split leads to invalid pointer dereference.
- [Medium] Missing NULL pointer check after btf__type_by_id allows segmentation fault on malformed BTF.
--

commit 2cf9036457ef4f9129e585e625be7f2e1a33c7fa
Author: Alan Maguire <alan.maguire@oracle.com>

selftests/bpf: Add a test verifying inline information

This commit adds a selftest to verify inline information for an
__always_inline function in bpf_testmod. It checks that the LOCSEC
descriptor offset is within the expected bounds of the function.

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_inline.c b/tools/testing/selftests/bpf/prog_tests/btf_inline.c
> index 0000000000000..8e0a85f6c6980
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_inline.c
[ ... ]
> +	inline_btf = btf__parse_split(BTF_SYSFS_DIR "/bpf_testmod" BTF_INLINE_SUFFIX,
> +				      btf);
> +	err = libbpf_get_error(inline_btf);
> +	/* pahole may not have inline BTF feature support. */
> +	if (err == -ENOENT) {
> +		skip = true;
> +		goto out;
> +	}
> +	locsec_id = btf__find_by_name_kind(inline_btf, ".text", BTF_KIND_LOCSEC);

[Severity: Medium]
Does this code handle errors other than -ENOENT correctly?

If btf__parse_split() fails with an error like -ENOMEM, err will be extracted,
but since it is not -ENOENT, execution continues and the unhandled ERR_PTR in
inline_btf is passed to btf__find_by_name_kind(), leading to a dereference of
an invalid pointer.

[ ... ]
> +	t = btf__type_by_id(inline_btf, l->loc_proto);
> +	if (!ASSERT_OK_PTR(t, "loc_proto_ptr"))
> +		goto out;
> +	if (!ASSERT_EQ(btf_vlen(t), 1, "loc_proto_one_param"))
> +		goto out;
> +	p = btf_loc_proto_params(t);
> +	t = btf__type_by_id(inline_btf, *p);
> +	lp = btf_loc_param(t);

[Severity: Medium]
Can this lead to a NULL pointer dereference on malformed BTF data?

Unlike the safe pattern used earlier with ASSERT_OK_PTR, the return value of
btf__type_by_id() is passed directly to btf_loc_param() and dereferenced via
lp->flags without a NULL check.

> +	if (!ASSERT_EQ(lp->flags, BTF_LOC_PARAM_REG, "param_is_reg"))
> +		goto out;
> +	if (!ASSERT_GT(l->offset, caller_addr, "inline_gt_caller"))
> +		goto out;
> +	/* simple sanity test to roughly ensure inline site still in function */
> +	if (ASSERT_LT(l->offset, caller_addr + 256, "inline_in_caller"))
> +		goto out;

[Severity: Medium]
Does this missing negation cause the test to exit early on success?

The ASSERT_LT macro evaluates to true when the check succeeds. Because the
condition lacks the negation ! used in all other assertions here, it will
execute the goto out branch when the sanity check passes.

> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index 679008b310d9a..bf4cb720e7b81 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
[ ... ]
> +long module_get_base_addr(const char *module)
> +{
> +	unsigned long long addr, base_addr = 0;
> +	char name[500];
> +	FILE *f;
> +
> +	f = fopen("/proc/modules", "r");
> +	if (!f)
> +		return 0;
> +
> +	while (fscanf(f, "%s %*s %*s %*s %*s 0x%llx", name, &addr) > 0) {

[Severity: Medium]
Can this parser become desynchronized if trailing fields are present in
/proc/modules?

The fscanf format string expects exactly 6 tokens and lacks a way to consume
the remainder of the line. Out-of-tree modules natively produce a 7th token
(such as the (O) taint flag). 

When this happens, the trailing token is not consumed and the next loop
iteration will mistakenly read it as the name, breaking the parsing structure
for all subsequent lines.

> +		if (strcmp(module, name) == 0) {
> +			base_addr = addr;
> +			break;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901165757.801449-1-alan.maguire@oracle.com?part=18