Re: [PATCH RFC 32/38] selftests/proc: check a sysctl listing while entries come and go

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/proc: missing prerequisite skips in sysctl_readdir_setup()
- [Low] selftests/proc: proc-sysctl-readdir-churn duplicates proc-readdir-churn
--

commit 9b75f87bfb2fb1adbf5b6ac83f3bfdac725df7f5
Author: Christian Brauner <brauner@kernel.org>

selftests/proc: check a sysctl listing while entries come and go

This commit adds a new kselftest binary, proc-sysctl-readdir-churn, to verify
that multi-call getdents64() listings of /proc/sys/net/ipv4/conf do not repeat
or skip entries when a tun network device is registered or unregistered
between calls.

> diff --git a/tools/testing/selftests/proc/proc-sysctl-readdir-churn.c b/tools/testing/selftests/proc/proc-sysctl-readdir-churn.c
> new file mode 100644
> index 0000000000000..0e067a3619c44
> --- /dev/null
> +++ b/tools/testing/selftests/proc/proc-sysctl-readdir-churn.c
> @@ -0,0 +1,147 @@

[ ... ]

> +#define CONF_DIR	"/proc/sys/net/ipv4/conf"
> +#define TUN_NAME	"a0"		/* sorts before "all" */
> +#define TUN_PATH	CONF_DIR "/" TUN_NAME
> +#define SMALL_BUF	80		/* ".", ".." and one short name */
> +#define NAMES		64
> +
> +static int tun_open(const char *name)
> +{
> +	struct ifreq ifr = {};
> +	int fd;
> +
> +	fd = open("/dev/net/tun", O_RDWR | O_CLOEXEC);
> +	if (fd < 0)
> +		return -1;
> +	strncpy(ifr.ifr_name, name, IFNAMSIZ - 1);
> +	ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
> +	if (ioctl(fd, TUNSETIFF, &ifr)) {
> +		close(fd);
> +		return -1;
> +	}
> +	return fd;
> +}
> +
> +/* one getdents64() into @buf, the names appended to @names */
> +static int list(int fd, char *buf, size_t size, char names[][NAME_MAX + 1],
> +		int *n)
> +{
> +	long ret, off;
> +	int got = 0;
> +
> +	ret = syscall(__NR_getdents64, fd, buf, size);
> +	if (ret < 0)
> +		return -errno;
> +	for (off = 0; off < ret;) {
> +		struct dirent64 *d = (void *)(buf + off);
> +
> +		if (*n < NAMES)
> +			strcpy(names[(*n)++], d->d_name);
> +		got++;
> +		off += d->d_reclen;
> +	}
> +	return got;
> +}
> +
> +static int count(char names[][NAME_MAX + 1], int n, const char *name)
> +{
> +	int i, c = 0;
> +
> +	for (i = 0; i < n; i++)
> +		if (!strcmp(names[i], name))
> +			c++;
> +	return c;
> +}
> +
> +FIXTURE(sysctl_readdir) {
> +	int dir;
> +	int n;
> +	char names[NAMES][NAME_MAX + 1];
> +	char buf[65536];
> +};

[Severity: Low]
This isn't a bug, but tun_open(), list(), count(), the fixture state in
FIXTURE(sysctl_readdir), and the register_in_front and unregister_in_front
test flows in tools/testing/selftests/proc/proc-sysctl-readdir-churn.c
duplicate the helpers, FIXTURE(proc_readdir), and the add_in_front and
remove_in_front test cases in tools/testing/selftests/proc/proc-readdir-churn.c,
differing only in the target directory (/proc/sys/net/ipv4/conf vs
/proc/net/dev_snmp6) and expected baseline entries (all, default, and lo vs
lo).

Could this be parameterized in proc-readdir-churn.c using FIXTURE_VARIANT and
FIXTURE_VARIANT_ADD instead of introducing a second standalone test file with
duplicated helpers and test logic?

> +
> +FIXTURE_SETUP(sysctl_readdir)
> +{
> +	if (unshare(CLONE_NEWNET))
> +		SKIP(return, "unshare(CLONE_NEWNET): %s", strerror(errno));
> +	if (access("/dev/net/tun", F_OK))
> +		SKIP(return, "/dev/net/tun: %s", strerror(errno));
> +	self->dir = open(CONF_DIR, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
> +	ASSERT_GE(self->dir, 0);
> +	self->n = 0;
> +}
> +
> +FIXTURE_TEARDOWN(sysctl_readdir)
> +{
> +	close(self->dir);
> +}
> +
> +/* a device registered in front of the position doesn't repeat an entry */
> +TEST_F(sysctl_readdir, register_in_front)
> +{
> +	int tun;
> +
> +	/* ".", ".." and "all", "default" doesn't fit */
> +	ASSERT_EQ(list(self->dir, self->buf, SMALL_BUF, self->names, &self->n), 3);
> +	ASSERT_EQ(count(self->names, self->n, "all"), 1);
> +
> +	tun = tun_open(TUN_NAME);
> +	ASSERT_GE(tun, 0);

[Severity: Medium]
In FIXTURE_SETUP(sysctl_readdir), opening CONF_DIR (/proc/sys/net/ipv4/conf)
is checked with ASSERT_GE(self->dir, 0), whereas proc-readdir-churn.c skips
when opening SNMP6_DIR fails:

tools/testing/selftests/proc/proc-readdir-churn.c:FIXTURE_SETUP(proc_readdir) {
	...
	self->dir = open(SNMP6_DIR, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
	if (self->dir < 0)
		SKIP(return, SNMP6_DIR ": %s", strerror(errno));
}

In addition, FIXTURE_SETUP(sysctl_readdir) only checks
access("/dev/net/tun", F_OK). On systems where /dev/net/tun exists as a
static device node (for example, created by udev or kmod static-nodes) while
CONFIG_TUN is disabled, access("/dev/net/tun", F_OK) succeeds, but
open("/dev/net/tun", ...) inside tun_open(TUN_NAME) fails with ENODEV or
ENXIO and triggers ASSERT_GE(tun, 0) in register_in_front and
unregister_in_front.

Since tools/testing/selftests/proc/config only enables CONFIG_PROC_FS=y at
this point, will running this test on a kernel without CONFIG_SYSCTL,
CONFIG_INET, or CONFIG_TUN fail the test instead of skipping when those
prerequisites are unavailable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-work-procfs-bugfixes-v1-0-59422ed3e465@kernel.org?part=32