Re: [PATCH v2 2/2] selftests/bpf: add test for blkcg io.stat BPF kfuncs

> diff --git a/tools/testing/selftests/bpf/cgroup_iter_io.h b/tools/testing/selftests/bpf/cgroup_iter_io.h
> new file mode 100644
> index 000000000000..f4bbaaccdf71
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/cgroup_iter_io.h

[ ... ]

> diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> index ea7044f30adc..270e6bf9194d 100644
> --- a/tools/testing/selftests/bpf/config
> +++ b/tools/testing/selftests/bpf/config

[ ... ]

> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter_io.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_io.c
> new file mode 100644
> index 000000000000..5d27e5d28379
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_io.c
> @@ -0,0 +1,277 @@

[ ... ]

> +/* Set up a loop device for cgroup-charged I/O. */
> +static int loop_setup(char *loop_path, size_t sz, int *ctl_fd, int *loop_fd,
> +		      int *back_fd)
> +{
> +	char back_path[] = "/tmp/cgroup_iter_io.XXXXXX";
> +	int nr;
> +
> +	*ctl_fd = *loop_fd = *back_fd = -1;
> +
> +	*ctl_fd = open("/dev/loop-control", O_RDWR | O_CLOEXEC);
> +	if (*ctl_fd < 0)
> +		return -1;
> +
> +	nr = ioctl(*ctl_fd, LOOP_CTL_GET_FREE);
> +	if (nr < 0)
> +		goto err;
> +	snprintf(loop_path, sz, "/dev/loop%d", nr);
> +
> +	*back_fd = mkstemp(back_path);
> +	if (*back_fd < 0)
> +		goto err;
> +	unlink(back_path);
> +	if (ftruncate(*back_fd, (off_t)IO_SIZE * 4))
> +		goto err;
> +
> +	*loop_fd = open(loop_path, O_RDWR | O_CLOEXEC);
> +	if (*loop_fd < 0)
> +		goto err;
> +	if (ioctl(*loop_fd, LOOP_SET_FD, *back_fd))
> +		goto err;
> +
> +	return 0;
> +err:
> +	if (*loop_fd >= 0)
> +		close(*loop_fd);
> +	if (*back_fd >= 0)
> +		close(*back_fd);
> +	close(*ctl_fd);
> +	*ctl_fd = *loop_fd = *back_fd = -1;
> +	return -1;
> +}

Does the err: cleanup path leak the loop device?

LOOP_CTL_GET_FREE can allocate a new loop device when no unbound device
exists, and nr is stored. Every failure after that point (mkstemp,
ftruncate, open(/dev/loopN), LOOP_SET_FD) leaves that device behind.

The caller jumps straight to cleanup_cgroup_fd on loop_setup() failure,
bypassing cleanup_loop, so loop_teardown() never runs and the device
remains allocated.

Recording nr and calling ioctl(*ctl_fd, LOOP_CTL_REMOVE, nr) on the err:
path would fix this. Additionally, loop_teardown() ignores the return of
both LOOP_CLR_FD and LOOP_CTL_REMOVE, so a device left in Lo_bound state
also leaks silently.

> +/* Read @dev's io.stat counters. @dev uses kernel dev_t encoding. */
> +static int parse_io_stat(int cgroup_fd, __u64 dev, struct io_query *out)
> +{
> +	unsigned int want_maj = dev >> 20, want_min = dev & ((1U << 20) - 1);
> +	char buf[4096], *line, *saveptr;
> +	int fd, n, ret = -1;
> +
> +	fd = openat(cgroup_fd, "io.stat", O_RDONLY);
> +	if (fd < 0)
> +		return -1;
> +	n = read(fd, buf, sizeof(buf) - 1);
> +	close(fd);
> +	if (n <= 0)
> +		return -1;
> +	buf[n] = '\0';

Will a single 4095-byte read() always capture the complete io.stat output?

io.stat is a seq_file with one line per blkg, and seq_read_iter() copies
with byte granularity, not record boundaries. Two failure modes:

1. If the cgroup's blkg list produces more than 4095 bytes, the loop
device's line may not be in the buffer, and parse_io_stat() returns -1.

2. The cut can land mid-line. A truncated line like "7:0 rbytes=4194304
wbytes=41" satisfies the '< 2' check below and is accepted with the
remaining counters silently zero, producing a bogus counter mismatch
against the kfunc rather than a parse error.

Reading in a loop until read() returns 0, or at least treating a non-
terminated final line as an error, would avoid both.

> +	for (line = strtok_r(buf, "\n", &saveptr); line;
> +	     line = strtok_r(NULL, "\n", &saveptr)) {
> +		unsigned long long rb = 0, wb = 0, ri = 0, wi = 0, db = 0, di = 0;
> +		unsigned int maj, min;
> +
> +		/* Only the device id is required; missing counters stay zero. */
> +		if (sscanf(line,
> +			   "%u:%u rbytes=%llu wbytes=%llu rios=%llu wios=%llu dbytes=%llu dios=%llu",
> +			   &maj, &min, &rb, &wb, &ri, &wi, &db, &di) < 2)
> +			continue;
> +		if (maj != want_maj || min != want_min)
> +			continue;
> +
> +		out->rbytes = rb;
> +		out->wbytes = wb;
> +		out->rios = ri;
> +		out->wios = wi;
> +		out->dbytes = db;
> +		out->dios = di;
> +		ret = 0;
> +		break;
> +	}
> +	return ret;
> +}

Should the partial conversion check be stricter?

Accepting '< 2' and then returning success means a line that matched the
device but whose counters were not parsed is reported as all-zero rather
than an error. The caller immediately uses strict equality checks:

	ASSERT_EQ(q->dbytes, filev.dbytes, "dbytes");

so a parse problem is reported as a counter mismatch against the kfunc,
pointing at the kernel instead of the parser.

This is reachable because blkcg_print_one_stat() emits the device name
unconditionally but gates the counter group with:

	if (rbytes || wbytes || rios || wios)
		seq_printf(s, "rbytes=%llu wbytes=%llu ... dios=%llu", ...);

A cgroup whose only traffic to a device was discard produces "MAJ:MIN "
with no key=value pairs. sscanf() returns 2, parse_io_stat() returns
success with dbytes=dios=0, and the match subtest fails on "dbytes" even
though the kernel is correct.

Requiring all eight conversions (== 8) once maj:min matches would surface
the real problem.

[ ... ]

> +	if (loop_setup(loop_path, sizeof(loop_path), &ctl_fd, &loop_fd, &back_fd)) {
> +		test__skip();	/* needs root + CONFIG_BLK_DEV_LOOP */
> +		goto cleanup_cgroup_fd;
> +	}

A subsystem pattern flags this as potentially concerning:

loop_setup() collapses eight distinct failures into a single -1, and the
caller turns every one of them into a bare test__skip() with no runtime
reason. The source comment says "needs root + CONFIG_BLK_DEV_LOOP", but
the same return value is produced by: open("/dev/loop-control") failing,
LOOP_CTL_GET_FREE failing, mkstemp() failing, ftruncate() failing (ENOSPC
on a small tmpfs /tmp), open("/dev/loopN") failing, and LOOP_SET_FD
failing.

The LOOP_SET_FD case is a genuine race: LOOP_CTL_GET_FREE is not a
reservation, so any concurrent consumer (another test_progs worker under
-j, udev, systemd, losetup) can bind /dev/loopN between GET_FREE and
SET_FD, and SET_FD then returns EBUSY. The result is that the whole test,
including all five subtests exercising the new blkcg kfuncs, silently
reports SKIP. Since nothing is printed, CI shows a green skip and a broken
kfunc goes unnoticed indefinitely.

Could the errno be printed before skipping, and "prerequisite absent"
(ENOENT/ENODEV/EPERM on /dev/loop-control) be distinguished from a real
failure (EBUSY, ENOSPC, ENOMEM)?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/cgroup_iter_io.c b/tools/testing/selftests/bpf/progs/cgroup_iter_io.c
> new file mode 100644
> index 000000000000..2cd538068987
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/cgroup_iter_io.c

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32073368069