Re: [PATCH net 2/2] selftests: net: netlink-dumps: start a dump with a notification queued

[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] The new test dump_with_queued_ntf in
  tools/testing/selftests/net/netlink-dumps.c needs privileges that the…
- [Low] dump_with_queued_ntf sets done = true as soon as it sees any
  NLMSG_DONE with nlmsg_seq == 1.

--

selftests: net: netlink-dumps: start a dump with a notification queued

This adds a test, dump_with_queued_ntf, to netlink-dumps.c. It starts an
RTM_GETLINK dump on a socket with a small receive buffer while a link
notification is still queued. The reader peeks before each read, the way
libnl does, and the test expects the dump to complete.

> diff --git a/tools/testing/selftests/net/netlink-dumps.c b/tools/testing/selftests/net/netlink-dumps.c
> index 51129c564d0a6..6f66a478d72f3 100644
> --- a/tools/testing/selftests/net/netlink-dumps.c
> +++ b/tools/testing/selftests/net/netlink-dumps.c

[ ... ]

> +// A dump started while a notification is still queued must make progress
> +// for a reader which peeks before reading, like libnl does
> +TEST(dump_with_queued_ntf)
> +{
> +	struct sockaddr_nl addr = {
> +		.nl_family	= AF_NETLINK,
> +		.nl_groups	= RTMGRP_LINK,
> +	};
> +	int netlink_sock, inet_sock, i;
> +	int rcvbuf = 8192;
> +	bool done = false;
> +	char buf[32768];
> +	ssize_t n;
> +
> +	ASSERT_EQ(unshare(CLONE_NEWNET), 0);

[Severity: Low]
Should this skip instead of fail when the test lacks privileges?

unshare(CLONE_NEWNET) needs CAP_SYS_ADMIN. lo_set_up() calls
ioctl(SIOCSIFFLAGS), which needs CAP_NET_ADMIN. Both results go through
ASSERT_EQ, so an unprivileged run reports FAIL rather than SKIP.

The other tests in this binary (dump_extack, test_sanity,
close_in_progress, close_with_ref) only open netlink sockets, send dump
requests and use mq_open/mq_notify. They already run as a normal user, so
this is the first test that makes the binary need privileges.

Would checking for errno == EPERM and calling SKIP() be enough here?

> +
> +	inet_sock = socket(AF_INET, SOCK_DGRAM, 0);
> +	ASSERT_GE(inet_sock, 0);

[ ... ]

> +		for (nlh = (struct nlmsghdr *)buf; NLMSG_OK(nlh, n);
> +		     nlh = NLMSG_NEXT(nlh, n)) {
> +			if (nlh->nlmsg_seq != dump_links.nlhdr.nlmsg_seq)
> +				continue;
> +			if (nlh->nlmsg_type == NLMSG_ERROR)
> +				EXPECT_EQ(*(int *)NLMSG_DATA(nlh), 0);
> +			if (nlh->nlmsg_type == NLMSG_DONE)
> +				done = true;

[Severity: Low]
Can this pass when the dump ends with an error?

netlink_dump_done() writes the final dump status into the NLMSG_DONE
payload:

net/netlink/af_netlink.c:netlink_dump_done() {
    ...
	memcpy(nlmsg_data(nlh), &nlk->dump_done_errno, sizeof(nlk->dump_done_errno));
    ...
}

If cb->dump() returns -EMSGSIZE with an empty skb, netlink_dump() keeps
the negative value:

net/netlink/af_netlink.c:netlink_dump() {
    ...
		if (nlk->dump_done_errno == -EMSGSIZE && skb->len)
			nlk->dump_done_errno = skb->len;
    ...
}

The dump then ends with an NLMSG_DONE that carries -EMSGSIZE. Other
negative dump errors end the same way.

A dump that starts successfully sends no separate NLMSG_ERROR ack. That
makes the NLMSG_DONE payload the only place the result is reported. This
loop checks the errno only for NLMSG_ERROR, so EXPECT_TRUE(done) still
passes. The test also never checks that an RTM_NEWLINK reply for lo
arrived.

The no-progress case this test targets is still caught, because no DONE
arrives there. The gap is a future regression in skb sizing that ends the
dump early with an error.

The existing nl_get_extack() helper in this file decodes the errno from
NLMSG_DONE as well:

	ea->err = -*(int *)NLMSG_DATA(nlh);

Could the new test check the NLMSG_DONE payload in the same way, and also
check that at least one RTM_NEWLINK message was received?

> +		}
> +	}
> +	EXPECT_TRUE(done);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-netlink-dump-livelock-v1-0-478214a635d2%40gmail.com