Re: [PATCH v3 bpf-next 6/7] selftests/bpf: Test BTF sysfs inline representations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] bpf: module_btf memory leak when .inline sysfs entry is missing
- [Low] bpf: missing continue statement after unexpected stat() failure leads to cascading failures
- [Low] bpf: partial memory leak on unexpected mmap success in btf sysfs test
--

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

selftests/bpf: Test BTF sysfs inline representations

This commit tests BTF sysfs inline representations by loading vmlinux,
vmlinux.inline, and all module/module.inline entries in /sys/fs/btf. This
verifies the kernel and module inline representations and sanity-checks
relocations.

There is a minor memory leak on an error path in test_btf_mmap_sysfs()
when the mmap with an invalid size unexpectedly succeeds:

tools/testing/selftests/bpf/prog_tests/btf_sysfs.c:test_btf_mmap_sysfs() {
    ...
	raw_data = mmap(NULL, end + 1, PROT_READ, MAP_PRIVATE, fd, 0);
	if (!ASSERT_EQ(raw_data, MAP_FAILED, "mmap_btf_invalid_size"))
		goto cleanup;
    ...
cleanup:
	btf__free(btf);
	if (raw_data && raw_data != MAP_FAILED)
		munmap(raw_data, btf_size);
    ...
}

[Severity: Low]
Does this leak the final page allocated by the end + 1 size?

If the oversized mmap unexpectedly succeeds, it allocates an extra page, but
the cleanup label only unmaps up to btf_size.

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_sysfs.c b/tools/testing/selftests/bpf/prog_tests/btf_sysfs.c
> index 3923e64c4c1d0..069c4483b3a21 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_sysfs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_sysfs.c

[ ... ]

> @@ -75,7 +80,78 @@ static void test_btf_mmap_sysfs(const char *path, struct btf *base)
>  		close(fd);
>  }
>  
> +static void test_btf_inline_sysfs_all(void)
> +{

[ ... ]

> +		if (!base_btf) {
> +			module_btf = btf__parse_split(btf_path, vmlinux_btf);
> +			err = libbpf_get_error(module_btf);
> +			if (err) {
> +				/* A module can be unloaded while its sysfs entry is iterated. */
> +				if (err == -ENOENT)
> +					continue;
> +				ASSERT_OK(err, "parse_module_btf");
> +				continue;
> +			}
> +			base_btf = module_btf;
> +		}
> +		if (stat(inline_path, &st)) {
> +			err = errno;
> +			if (err == ENOENT)
> +				continue;

[Severity: Medium]
Does this early continue leak the memory allocated for module_btf?

When an .inline file does not exist, the continue statement bypasses the
btf__free(module_btf) call at the bottom of the loop. Since most modules
might lack this representation, this could leak memory for every module.

> +			ASSERT_OK(err, "stat_inline_btf");
> +		}

[Severity: Low]
Should there be a continue statement here to avoid cascading failures?

If stat() fails with an error other than ENOENT, the code asserts but falls
through to parse the nonexistent or inaccessible inline file instead of
jumping to the next iteration.

> +		if (base_btf == vmlinux_btf)
> +			test_btf_mmap_sysfs(inline_path, base_btf);
> +		inline_btf = btf__parse_split(inline_path, base_btf);
> +		err = libbpf_get_error(inline_btf);
> +		if (!err)
> +			btf__free(inline_btf);
> +		ASSERT_OK(err, "parse_inline_btf");
> +		btf__free(module_btf);
> +	}
> +	closedir(dir);
> +
> +	btf__free(vmlinux_btf);
> +}

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