Re: [PATCH net 4/4] selftest: af_unix: Test SIOCINQ and scm_fds with OOB and dgram disconnect.

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Two new tests in
  tools/testing/selftests/net/af_unix/scm_rights.c, oob_fdinfo() and…

--

selftest: af_unix: Test SIOCINQ and scm_fds with OOB and dgram disconnect.

This commit adds a SIOCINQ check to msg_oob's __resetpair() so that the
drained stream receive queue reports 0 unread bytes. It also adds
oob_fdinfo and dgram_disconnect_fdinfo tests to scm_rights.c. These check
that scm_fds in /proc/self/fdinfo/<fd> goes back to 0 when OOB skbs carrying
SCM_RIGHTS are skipped or freed, and when a dgram socket with queued
SCM_RIGHTS skbs disconnects.

> diff --git a/tools/testing/selftests/net/af_unix/scm_rights.c b/tools/testing/selftests/net/af_unix/scm_rights.c
> index c165f250220a4..38a125d9f6e49 100644
> --- a/tools/testing/selftests/net/af_unix/scm_rights.c
> +++ b/tools/testing/selftests/net/af_unix/scm_rights.c

[ ... ]

> @@ -395,4 +425,82 @@ TEST_F(scm_rights, mixed_lowpoint)
>  	close_sockets(6);
>  }
>  
> +TEST_F(scm_rights, oob_fdinfo)
> +{
> +	int recv_fd, ret;
> +	char buf[2];
> +
> +	if (!(variant->flags & MSG_OOB) || variant->disabled)
> +		return;

[Severity: Low]
Should these early returns use SKIP() instead?

In kselftest_harness, a plain return from a TEST_F body counts as a pass.
The scm_rights fixture has 10 variants, and oob_fdinfo() only does real
work on stream_oob and stream_listener_oob. The other 8 variants report
"ok" even though nothing was checked.

dgram_disconnect_fdinfo() later in this patch has the same pattern:

	if (variant->type != SOCK_DGRAM || variant->disabled)
		return;

It only runs on dgram and reports "ok" on the other 9 variants. That makes
17 TAP results that show as passes rather than "# SKIP".

The sibling test scm_rights_denial_lsm.c already uses this form:

	SKIP(return, "requires root");

Also, FIXTURE_SETUP and FIXTURE_TEARDOWN still run for these no-op cases.
FIXTURE_TEARDOWN(scm_rights) does this:

	if (variant->disabled)
		return;

	sleep(1);

So the no-op runs on non-disabled variants add about 7 seconds of runtime.
That is 3 for oob_fdinfo (dgram, stream, stream_listener) and 4 for
dgram_disconnect_fdinfo (stream, stream_oob, stream_listener,
stream_listener_oob).

> +
> +	create_sockets(2);
> +
> +	if (variant->test_listener) {
> +		recv_fd = accept(self->fd[0], NULL, NULL);
> +		ASSERT_LE(0, recv_fd);
> +	} else {
> +		recv_fd = self->fd[0];
> +	}

[ ... ]

> +TEST_F(scm_rights, dgram_disconnect_fdinfo)
> +{
> +	struct sockaddr addr = {
> +		.sa_family = AF_UNSPEC,
> +	};
> +	int ret;
> +
> +	if (variant->type != SOCK_DGRAM || variant->disabled)
> +		return;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106051905.218638-1-sidchintamaneni%40gmail.com