Re: [PATCH net v2 3/3] selftests: net: netlink-dumps: test dump of route larger than skb

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] dump_large_route() adds an IPv4 multipath route with
  BIG_RT4_NH = 1500 gateway-less nexthops.
- [Low] In tools/testing/selftests/net/netlink-dumps.c, nl_request_ack()
  cannot decode an error ACK for either of the two large RTM_NEWROUTE…
- [Low] The comment in dump_large_route() before close(netlink_sock) says
  "The ACK reads above used large buffers, which raise the dump skb size…

--

selftests: net: netlink-dumps: test dump of route larger than skb

This commit adds a dump_large_route test. The test installs IPv4 and
IPv6 multipath routes that are too large for the first dump skb of a
fresh socket. It then checks that the dump either returns the route or
fails with -EMSGSIZE.

The commit also creates a dummy device for the IPv6 nexthops,
generalizes lo_set_up() to if_set_up(), and adds the IPv4 routing
options to the config.

> diff --git a/tools/testing/selftests/net/netlink-dumps.c b/tools/testing/selftests/net/netlink-dumps.c
> index 4a94a25167e25..43f63f3f22895 100644
> --- a/tools/testing/selftests/net/netlink-dumps.c
> +++ b/tools/testing/selftests/net/netlink-dumps.c

[ ... ]

> @@ -362,4 +362,267 @@ TEST(dump_with_queued_ntf)

[ ... ]

> +static int nl_request_ack(int sock, const void *req, size_t len)
> +{
> +	struct nlmsghdr *nlh;
> +	char buf[4096];
> +	ssize_t n;
> +
> +	if (send(sock, req, len, 0) != (ssize_t)len)
> +		return -errno;
> +	n = recv(sock, buf, sizeof(buf), 0);
> +	if (n < 0)
> +		return -errno;
> +	nlh = (struct nlmsghdr *)buf;
> +	if (!NLMSG_OK(nlh, n) || nlh->nlmsg_type != NLMSG_ERROR)
> +		return -EPROTO;

[Severity: Low]
Can nl_request_ack() report the real errno when the kernel rejects one
of the large RTM_NEWROUTE requests?

On error, netlink_ack() copies the whole original request into the
reply unless NETLINK_CAP_ACK is set, and this test never sets it:

net/netlink/af_netlink.c:netlink_ack() {
    ...
	if (err && !test_bit(NETLINK_F_CAP_ACK, &nlk->flags))
		payload += nlmsg_len(nlh);
    ...
}

new_route4 is 48 + 1500 * 8 = 12048 bytes and new_route6 is
60 + 500 * 28 = 14060 bytes, so neither error ACK fits in buf[4096].
recv() truncates the datagram and returns 4096. NLMSG_OK() then fails
because nlmsg_len > n, and the helper returns -EPROTO without reading
the errno at NLMSG_DATA(nlh).

The ASSERT_EQ() calls in dump_large_route() still fail when they
should. But every route-add failure (-ENOBUFS, -EINVAL, -ENETUNREACH,
and so on) gets printed as -71.

Would setting NETLINK_CAP_ACK on the socket fix this? Another option is
to read with MSG_PEEK | MSG_TRUNC into a buffer big enough for the
copied request.

> +	return *(int *)NLMSG_DATA(nlh);
> +}

[ ... ]

> +TEST(dump_large_route)
> +{
> +	int netlink_sock, inet_sock, i, ret, routes;

[ ... ]

> +	for (i = 0; i < BIG_RT4_NH; i++) {
> +		new_route4.nh[i].rtnh_len = sizeof(new_route4.nh[i]);
> +		new_route4.nh[i].rtnh_ifindex = 1; // lo
> +	}
> +	ASSERT_EQ(nl_request_ack(netlink_sock, &new_route4,
> +				 sizeof(new_route4)), 0);

[Severity: Medium]
Could this route add fail spuriously on a machine with fragmented
memory?

With BIG_RT4_NH = 1500, fib_create_info() allocates the fib_info and
all 1500 fib_nh entries as one contiguous block:

net/ipv4/fib_semantics.c:fib_create_info() {
    ...
	fi = kzalloc_flex(*fi, fib_nh, nhs);
    ...
}

kzalloc_flex() expands to plain kzalloc(), not kvzalloc(). struct
fib_nh is about 112-120 bytes on 64-bit, so the request is about
170-180 KB. That is an order-6 allocation on 4K-page kernels.

If that allocation fails, fib_create_info() returns -ENOBUFS. Because
__GFP_NOWARN is not set, the kernel also logs a page allocation failure
warning in dmesg. The test then fails for reasons unrelated to the dump
behaviour it checks.

The count is this high only because each gateway-less IPv4 nexthop adds
just 8 bytes to the dumped route. Could each IPv4 nexthop get an
RTA_GATEWAY, as the IPv6 nexthops do? That would reach the same dump
size with far fewer kernel-side fib_nh entries.

[ ... ]

> +	ASSERT_EQ(nl_request_ack(netlink_sock, &new_route6,
> +				 sizeof(new_route6)), 0);
> +
> +	// The ACK reads above used large buffers, which raise the dump
> +	// skb size for this socket. Dump on fresh sockets instead.
> +	close(netlink_sock);

[Severity: Low]
Is this comment accurate? The ACK reads in nl_request_ack() use a
4096-byte buffer, so max_recvmsg_len for this socket only reaches 4096.

netlink_dump() uses max_recvmsg_len only when it exceeds
alloc_min_size:

net/netlink/af_netlink.c:netlink_dump() {
    ...
	alloc_min_size = max_t(int, cb->min_dump_alloc, NLMSG_GOODSIZE);

	max_recvmsg_len = READ_ONCE(nlk->max_recvmsg_len);
	if (alloc_min_size < max_recvmsg_len) {
    ...
}

On 4K-page kernels NLMSG_GOODSIZE is SKB_WITH_OVERHEAD(4096), so the
dump skb grows by only a few hundred bytes. With 8K or larger pages,
nothing changes. Either way, the 12 KB and 14 KB routes would still not
fit in the first dump skb if this socket were reused.

Opening fresh sockets is fine, but could the comment be reworded? The
buffer that actually raises later dump skb sizes is the 32 KB one in
dump_big_table().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110172925.3293312-1-stephen%40networkplumber.org