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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: module_get_base_addr() desyncs on trailing /proc/modules tokens
- [Medium] bpf: btf_inline test incorrectly allows memory-spilled parameter
--

commit 4adb006744e35941ca2b357431675f3202d1100f
Author: Alan Maguire <alan.maguire@oracle.com>
Subject: selftests/bpf: Add a test verifying inline information

This commit adds a selftest to verify inline information for bpf_testmod.
It checks that an always-inlined function has a single location parameter
encoded in a register, and that the LOCSEC descriptor offset is within the
expected range.

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_inline.c b/tools/testing/selftests/bpf/prog_tests/btf_inline.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_inline.c
[ ... ]
> +	lp = btf_loc_param(t);
> +	if (!ASSERT_TRUE(lp->flags & BTF_LOC_PARAM_REG, "param_has_reg"))
> +		goto out;

[Severity: Medium]
Because the variable in testmod_inline_len() below is forced into memory, its
BTF location is encoded as a register-relative memory offset, which sets both
BTF_LOC_PARAM_REG and BTF_LOC_PARAM_OFFSET.

Does this bitwise AND accidentally pass for memory offsets since they have a
base register? If so, this check might be masking the fact that the parameter
was actually spilled to the stack instead of being purely encoded in a
register as the commit message describes.

[ ... ]
> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> index 93847ca6293b4..b4f0af2aeca45 100644
> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
[ ... ]
> @@ -735,6 +735,13 @@ bpf_testmod_test_read(struct file *file, struct kobject *kobj,
>  EXPORT_SYMBOL(bpf_testmod_test_read);
>  ALLOW_ERROR_INJECTION(bpf_testmod_test_read, ERRNO);
>  
> +static __always_inline size_t testmod_inline_len(size_t len)
> +{
> +	/* Keep an instruction in the inline site without changing len. */
> +	asm volatile("" : "+m"(len));

[Severity: Medium]
Does the "+m" constraint here force the len variable into memory rather
than a register? The commit message explicitly states the intent is to verify
a parameter "encoded in a register", but this constraint forces a stack spill
instead.

> +	return len;
> +}
[ ... ]
> diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c
> index 679008b310d9a..f3a1d627699da 100644
> --- a/tools/testing/selftests/bpf/trace_helpers.c
> +++ b/tools/testing/selftests/bpf/trace_helpers.c
[ ... ]
> +unsigned long 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, "%499s %*s %*s %*s %*s 0x%llx", name, &addr) == 2) {

[Severity: Medium]
Will this loop terminate prematurely if it encounters trailing data on a line
in /proc/modules?

If a module (like an out-of-tree or unsigned module) has an optional trailing
taint string such as "(O)", this fscanf format lacks a line consumer (like
%*[^\n]\n) to read to the end of the line.

On the next iteration, the %499s token will mistakenly consume the leftover
taint string instead of the next module's name, shifting all subsequent fields
by one token. When the 0x%llx field then attempts to parse the state string
(e.g., "Live"), it will fail to match the "0x" prefix and return 1, causing
the loop to exit early and return 0 instead of the correct base address.

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

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