Re: [PATCH 3/5] selftests/bpf: add iocost cost model test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] selftests/bpf: unused any variable and false test skip in iocost_bpf_supported()
- [Medium] selftests/bpf: missing MS_PRIVATE in iocost_test_env() leaks configfs mounts
- [Medium] selftests/bpf: iocost_model attaches to iocost_ioc_tick before it is added
- [Low] selftests/bpf: missing bpf_link__destroy() in serial_test_iocost_model()
- [Low] selftests/bpf: missing rmdir() on error paths in nullb_create()
--

commit 6b66848245188ed8cc601a3e3e3fb4d06a7f79b3
Author: Tao Cui <cuitao@kylinos.cn>

selftests/bpf: add iocost cost model test

This commit adds two example BPF iocost cost models (a 2x linear HDD model
and a multi-stream sequentiality model) along with selftests exercising
model attachment, switching, pricing, tracepoints, and device removal using
null_blk via configfs.

> diff --git a/tools/testing/selftests/bpf/prog_tests/iocost_model.c b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
> new file mode 100644
> index 0000000000000..5373d52d93a66
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
[ ... ]
> +static int nullb_create(const char *name, char *dev, size_t dev_sz)
> +{
> +	char path[128];
> +	char p2[128];
> +	int err;
> +
> +	snprintf(path, sizeof(path), "/sys/kernel/config/nullb/%s", name);
> +	if (mkdir(path, 0755)) {
[ ... ]
> +	}
> +
> +	snprintf(path, sizeof(path), "/sys/kernel/config/nullb/%s/size", name);
> +	err = configfs_write(path, "128");
> +	if (err)
> +		return err;
> +
> +	snprintf(path, sizeof(path), "/sys/kernel/config/nullb/%s/power", name);
> +	err = configfs_write(path, "1");
> +	if (err)
> +		return err;

[Severity: Low]
Does nullb_create() leak the configfs directory and its underlying
struct nullb_device if writing to the size or power attribute fails?

When mkdir() creates /sys/kernel/config/nullb/<name>, configfs allocates the
device item via nullb_group_make_group(). If either configfs_write() call for
size or power fails, nullb_create() returns early without calling rmdir(path)
(unlike the read_file_token() failure path below), and callers such as
serial_test_iocost_model() and serial_test_iocost_model_streams() return
immediately without calling nullb_remove().

Should these error paths also remove the created configfs directory before
returning?

[ ... ]
> +static bool iocost_bpf_supported(void)
> +{
> +	FILE *f;
> +	char line[128];
> +	bool any = false, on = false;
> +
> +	f = popen("zcat /proc/config.gz 2>/dev/null", "r");
> +	if (!f)
> +		return false;
> +	while (fgets(line, sizeof(line), f)) {
> +		if (strstr(line, "CONFIG_"))
> +			any = true;
> +		if (strstr(line, "CONFIG_BLK_CGROUP_IOCOST_BPF=y"))
> +			on = true;
> +	}
> +	pclose(f);
> +	return on;
> +}

[Severity: Medium]
Will iocost_bpf_supported() falsely skip all tests in this file when
/proc/config.gz is not available?

In iocost_bpf_supported(), the local variable any is set when configuration
lines are read from /proc/config.gz, but any is never used in the return
statement (which also triggers an unused-but-set variable warning). Because
tools/testing/selftests/bpf/config does not enable ikconfig in procfs,
popen() succeeds while fgets() reads zero lines when /proc/config.gz or zcat
is absent, leaving both any and on as false.

As a result, iocost_test_env()->iocost_bpf_supported() returns false and skips
serial_test_iocost_model(), serial_test_iocost_model_streams(), and
serial_test_iocost_model_removal() even when the running kernel has iocost
BPF support enabled.

Was this meant to return !any || on, or check for struct iocost_model_ops in
vmlinux BTF instead?

> +
> +static bool iocost_test_env(void)
> +{
[ ... ]
> +	/* isolate the configfs and cgroup mounts from the host */
> +	if (unshare(CLONE_NEWNS)) {
> +		printf("%s:SKIP:unshare(CLONE_NEWNS) failed: %s\n",
> +		       __func__, strerror(errno));
> +		test__skip();
> +		return false;
> +	}
> +	mkdir("/sys/kernel/config", 0755);
> +	(void)mount("none", "/sys/kernel/config", "configfs", 0, NULL);

[Severity: Medium]
Can this configfs mount in iocost_test_env() propagate back into the host
mount namespace and accumulate across tests?

On systems where / or /sys uses shared mount propagation (the systemd
default), calling unshare() with a new mount namespace followed immediately by
mount() without first setting private mount propagation:

    mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL);

causes the new /sys/kernel/config mount to propagate back to the host mount
namespace. In addition, because serial_test_iocost_model(),
serial_test_iocost_model_streams(), and serial_test_iocost_model_removal()
each call iocost_test_env() without unmounting on teardown, three stacked
configfs mounts accumulate on /sys/kernel/config.

[ ... ]
> +void serial_test_iocost_model(void)
> +{
[ ... ]
> +		/*
> +		 * detach implicitly by closing the link fd instead of
> +		 * calling bpf_link__destroy() explicitly: the release
> +		 * path must detach the model just like the explicit
> +		 * call does
> +		 */
> +		lfd = bpf_link__fd(skel->links.iocost_2x);
> +		bpf_link__disconnect(skel->links.iocost_2x);
> +		close(lfd);
> +		skel->links.iocost_2x = NULL;

[Severity: Low]
Does setting skel->links.iocost_2x = NULL here without calling
bpf_link__destroy() leak the struct bpf_link_struct_ops allocation?

In tools/lib/bpf/libbpf.c, bpf_link__disconnect() only marks the link as
disconnected so that bpf_link__destroy() will skip calling link->detach():

    void bpf_link__disconnect(struct bpf_link *link)
    {
        link->disconnected = true;
    }

Clearing skel->links.iocost_2x to NULL before bpf_link__destroy() or
iocost_model__destroy(skel) runs prevents link->dealloc() from freeing the
heap-allocated struct bpf_link_struct_ops created by
bpf_map__attach_struct_ops(), which triggers a LeakSanitizer failure under
address sanitizer builds.

> diff --git a/tools/testing/selftests/bpf/progs/iocost_model.c b/tools/testing/selftests/bpf/progs/iocost_model.c
> new file mode 100644
> index 0000000000000..8875fb0f44174
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/iocost_model.c
[ ... ]
> +/* consumes the new iocost_ioc_tick tracepoint from BPF side */
> +SEC("tp_btf/iocost_ioc_tick")
> +__u64 BPF_PROG(iocost_2x_tick_count, struct ioc *ioc,
> +	       const struct ioc_now *now, int nr_active, u64 usage_us_sum)
> +{
> +	ioc_ticks++;
> +	return 0;
> +}

[Severity: Medium]
Does iocost_model__load() fail at this commit because the iocost_ioc_tick
tracepoint has not been introduced yet?

Here iocost_2x_tick_count() is defined with SEC("tp_btf/iocost_ioc_tick") and
is autoloaded when serial_test_iocost_model() and
serial_test_iocost_model_removal() call iocost_model__load(), and
serial_test_iocost_model() also asserts that skel->bss->ioc_ticks is greater
than zero.

However, the iocost_ioc_tick tracepoint in include/trace/events/iocost.h and
block/blk-iocost.c is only added in the next patch in the series ("blk-iocost:
add iocost_ioc_tick tracepoint for per-period device summary"), so loading the
skeleton at this point in the series fails in libbpf_find_attach_btf_id() When
looking up btf_trace_iocost_ioc_tick.

Should the patch adding the iocost_ioc_tick tracepoint come before this
selftest patch so bisection works cleanly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028143534.135968-1-cui.tao@linux.dev?part=3