Re: [RFC PATCH 6/8] selftests/bpf: add iocost cost model test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The negative test case incorrectly checks the return value of `fprintf` instead of `fclose` or `fflush`, causing the test to assert the wrong condition and spuriously fail.
- [Medium] Interleaved input and output on the same stream in update mode without an intervening file positioning function causes undefined behavior.
- [Low] Several multi-line comments do not follow the mandatory BPF subsystem multi-line comment style.
--

commit 312862fd426ea5bb64f752670a814d2e20308af1
Author: Tao Cui <cuitao@kylinos.cn>

selftests/bpf: add iocost cost model test

This commit adds an example cost model implementing iocost_model_ops and a 
runner that registers it as a struct_ops. The runner verifies readback and 
ensures that writing to the cost model is properly rejected if no model is 
registered.

> 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..386026241d5b4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
> @@ -0,0 +1,124 @@
[ ... ]
> +static int switch_to_bpf(const char *dev)
> +{
> +	char path[] = "/sys/fs/cgroup/io.cost.model";
> +	char word[256], line[256], buf[300];
> +	FILE *fp;
> +	int err, found = 0;
> +
> +	fp = fopen(path, "r+");
> +	if (!fp) {
> +		TH_LOG("open %s: %s", path, strerror(errno));
> +		return -1;
> +	}
> +
> +	while (fgets(line, sizeof(line), fp)) {
> +		if (sscanf(line, "%255s", word) == 1 && !strcmp(word, dev)) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +	if (!found) {
> +		TH_LOG("%s has no iocost line", dev);
> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	snprintf(buf, sizeof(buf), "%s ctrl=bpf", dev);
> +	err = fprintf(fp, "%s\n", buf);

[Severity: Medium]
Is an intervening file positioning function like fseek() or fflush() needed 
here before calling fprintf()?

The C11 standard mandates an intervening call to a file positioning function 
or fflush() between reading and writing when a stream is opened in update 
mode ("r+"). Without it, stream buffer pointers can become corrupted, causing 
the write to fail, be dropped, or write at an incorrect offset.

> +	if (err <= 0) {
> +		TH_LOG("write ctrl=bpf failed");
> +		fclose(fp);
> +		return -1;
> +	}
> +	fflush(fp);
> +
> +	rewind(fp);
> +	found = 0;
> +	while (fgets(line, sizeof(line), fp)) {
> +		if (sscanf(line, "%255s", word) == 1 && !strcmp(word, dev)) {
> +			found = strstr(line, "ctrl=bpf") != NULL;
> +			break;
> +		}
> +	}
> +	if (!found)
> +		TH_LOG("readback does not show ctrl=bpf");
> +
> +	fprintf(fp, "%s ctrl=auto\n", dev);

[Severity: Medium]
Similarly, is an fseek() or fflush() needed here between the fgets() read
loop and this fprintf() write to avoid undefined behavior?

> +	fclose(fp);
> +	return found ? 0 : -1;
> +}
[ ... ]
> +void serial_test_iocost_model(void)
> +{
> +	struct iocost_model *skel;
> +	char word[256], *dev;
> +	int err;
> +
> +	dev = getenv("IOCOST_TEST_DEV");
> +	if (!dev || geteuid() != 0) {
> +		test__skip();
> +		return;
> +	}
> +	if (!ASSERT_TRUE(dev_has_iocost(), "iocost_mounted"))
> +		return;
> +
> +	/* negative: ctrl=bpf must be rejected while no model is
> +	 * registered, so a typo cannot silently disable cost model
> +	 * updates */

[Severity: Low]
This isn't a bug, but does this violate the BPF subsystem multi-line comment
style?

The BPF guidelines specify that the opening /* should be on its own line:

/*
 * negative: ctrl=bpf must be rejected while no model is
 * registered...

> +	if (sscanf(dev, "%255s", word) != 1)
> +		return;
> +	{
> +		char path[] = "/sys/fs/cgroup/io.cost.model";
> +		char buf[300];
> +		FILE *fp = fopen(path, "w");
> +
> +		snprintf(buf, sizeof(buf), "%s ctrl=bpf\n", word);
> +		err = 0;
> +		if (fp) {
> +			err = fprintf(fp, "%s", buf) <= 0;
> +			fclose(fp);
> +		}
> +		ASSERT_TRUE(err, "ctrl_bpf_without_model_rejected");

[Severity: Medium]
Should this error check evaluate the return value of fclose() or fflush()
instead of fprintf()?

Since fopen() on cgroupfs files defaults to full buffering, fprintf() will
merely copy data to the userspace buffer and return success. The actual
rejection from the kernel (-EINVAL) occurs during the write() syscall
triggered by fclose(). Checking fprintf() here might cause the test to assert
the wrong condition and spuriously fail.

> +	}
> +
> +	skel = iocost_model__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open_load"))
> +		return;
> +
> +	/* attaching the struct_ops registers the model; a second
> +	 * registration of the same ops would fail with EBUSY */

[Severity: Low]
This isn't a bug, but this comment also doesn't follow the preferred BPF
multi-line comment format where the /* sits on its own line.

> +	err = iocost_model__attach(skel);
> +	if (ASSERT_OK(err, "attach"))
> +		ASSERT_OK(switch_to_bpf(dev), "switch_and_readback");
> +
> +	iocost_model__destroy(skel);
> +}
> 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..70f7987f8eb6a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/iocost_model.c
> @@ -0,0 +1,91 @@
[ ... ]
> +#define IOC_PAGE_SIZE		4096
> +#define IOC_SECT_TO_PAGE_SHIFT	3	/* 512B sectors to 4kB pages */
> +#define LCOEF_RANDIO_PAGES	4096	/* 16MB seek threshold */
> +#define IOCOST_COST_F_MERGE	(1ULL << 0)	/* not in BTF: a plain macro */
> +
> +/* DIV64_U64_ROUND_UP / DIV_ROUND_UP_ULL equivalents, folded at
> + * compile time */

[Severity: Low]
This isn't a bug, but this multi-line comment has the opening /* on the same
line as the text, which isn't the preferred style for the BPF subsystem.

> +#define RU(x, y)		((x) / (y) + (((x) % (y)) ? 1 : 0))
> +
> +#define RBPS	174019176ULL
> +#define RSEQIOPS	41708ULL

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