> 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
