Re: [PATCH net v2 1/2] netlink: check sk_rcvbuf before charging the dump skb

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (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).

- [High] In netlink_dump() (net/netlink/af_netlink.c:2327), a dump can
  still fail to start because of memory charged by skbs that have…
- [Low] The new comment in netlink_dump() ("netlink_recvmsg() continues
  the dump at or below half of sk_rcvbuf, and the skb must be queued…
- [Low] The dump_extack selftest (tools/testing/selftests/net/netlink-
  dumps.c, comment at lines 158-161, branch at 162-163) still describes…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it has the
  same trigger the commit describes.

--

netlink: check sk_rcvbuf before charging the dump skb

This patch changes netlink_dump() so that sk_rmem_alloc is compared
against sk_rcvbuf before the new dump skb is charged instead of after.
A dump can then start while something else is still charged to the
socket. It also refuses an skb that would push sk_rmem_alloc past
INT_MAX, so the wraparound fix from ae8f160e7eb2 still holds.

> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index a904dc3bcad6b..4129e93a9ff0c 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2318,9 +2318,13 @@ static int netlink_dump(struct sock *sk, bool lock_taken)
>  	if (!skb)
>  		goto errout_skb;
>  
> +	/* Check the limit before charging the skb: netlink_recvmsg() continues
> +	 * the dump at or below half of sk_rcvbuf, and the skb must be queued
> +	 * then. Keep sk_rmem_alloc at or below INT_MAX.
> +	 */

[Severity: Low]
Is "the skb must be queued then" guaranteed? netlink_recvmsg() checks its
gate without any lock that keeps senders out:

netlink_recvmsg() {
    ...
	if (READ_ONCE(nlk->cb_running) &&
	    atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2) {
		ret = netlink_dump(sk, false);
    ...
}

netlink_dump() then takes nl_cb_mutex and calls alloc_skb(GFP_KERNEL).
Both can sleep before the skb is charged. Several things can happen in
that window:

  - netlink_broadcast_deliver() fills the socket up to rcvbuf, or puts
    an skb of any truesize into an empty socket.
  - A rejected broadcast briefly over-charges before its atomic_sub().
  - setsockopt(SO_RCVBUF) shrinks rcvbuf.
  - A second reader's dump skb gets queued first.

Any of these can make rmem - skb->truesize >= rcvbuf true after the gate
passed. netlink_recvmsg() then sets sk_err while the dump keeps running.
A later read seems to resume the dump, so this looks transient. The same
races also existed before this patch.

Could the comment and the commit message ("so the skb is then always
queued") be softened to say this only holds when nothing else charges
the socket concurrently?

Separately, the plain read of sk->sk_rcvbuf in that gate races with the
WRITE_ONCE() in sock_setsockopt(). That part is pre-existing.

>  	rcvbuf = READ_ONCE(sk->sk_rcvbuf);
>  	rmem = atomic_add_return(skb->truesize, &sk->sk_rmem_alloc);
> -	if (rmem != skb->truesize && rmem >= rcvbuf) {
> +	if (rmem - skb->truesize >= rcvbuf || rmem > INT_MAX) {

[Severity: High]
Can the dump still get stuck when the stale charge alone is at least
sk_rcvbuf?

The commit message says this fixes "A dump that starts while something
else is still charged to a socket with a small receive buffer fails with
ENOBUFS and stays running". This check only covers a stale charge that is
below sk_rcvbuf.

netlink_broadcast_deliver() accepts a notification of any truesize into
a socket with nothing charged, through rmem == skb->truesize. The
selftest in the next patch uses SO_RCVBUF 2048, which gives sk_rcvbuf
4096. With a buffer that small, one rtnetlink or genetlink notification
can have truesize >= sk_rcvbuf.

do_one_broadcast() gives the first listener the original skb through
p->skb2 = skb_get(p->skb). After the reader dequeues it, the charge stays
on the socket until netlink_broadcast_filtered() calls consume_skb(skb).
This is the preempted-sender state from the commit message. The
broadcaster can also sleep in skb_clone() for later listeners.

The reader then sends a dump request:

__netlink_dump_start()
    WRITE_ONCE(nlk->cb_running, true);
    netlink_dump()
        rmem - skb->truesize >= rcvbuf -> -ENOBUFS
    return ret;   /* cb_running is still true */

The ack goes through netlink_rcv_skb()->netlink_ack()->netlink_unicast()
->netlink_attachskb(). There rmem = stale + ack truesize, which is above
rcvbuf and not equal to the ack truesize, so the ack is dropped.
netlink_overrun() then sets sk_err, or does nothing if NETLINK_NO_ENOBUFS
is set.

When the broadcaster later drops its reference, sock_rfree() only
uncharges. It does not wake the reader or call netlink_dump().

netlink_recvmsg() resumes a dump only after skb_recv_datagram() returns
an skb, and the queue is empty. So:

  - the next recvmsg() returns sk_err;
  - the one after that blocks;
  - every later dump request gets -EBUSY from __netlink_dump_start(),
    until some unrelated message arrives.

Would it be more robust to never fail the first netlink_dump() call from
__netlink_dump_start() because of rcvbuf? Another option is to clear
cb_running, or arrange a resume, when that call fails.

[Severity: Low]
This isn't a bug, but the dump_extack test in
tools/testing/selftests/net/netlink-dumps.c still describes the old
behaviour:

dump_extack() {
    ...
		/* Once we fill the buffer we'll see one ENOBUFS followed
		 * by a number of EBUSYs. Then the last recv() will finally
		 * trigger and complete the dump.
		 */
		if (ret == FOUND_ERR && (ea.err == ENOBUFS || ea.err == EBUSY))
			continue;
    ...
}

With this check, the bad RTM_GETNEIGH dumps keep queueing DONE+extack
skbs as long as the charge before each one is below sk_rcvbuf. So
sk_rmem_alloc ends up in [rcvbuf, rcvbuf + truesize).

The ENOBUFS ack for the next dump is then refused in netlink_attachskb(),
and netlink_overrun() sets NETLINK_S_CONGESTED. Because of that bit, the
later EBUSY acks are dropped as well. The ENOBUFS/EBUSY branch above can
no longer be reached.

Should the comment and the branch be updated, perhaps to check the new
behaviour instead? The next patch in the series only adds a new test and
leaves dump_extack unchanged.

[Severity: Medium]
This isn't a bug introduced by this patch, but the same trigger can also
stall a dump after it has started. netlink_recvmsg() is the only place
that continues a running dump:

netlink_recvmsg() {
    ...
	skb_free_datagram(sk, skb);

	if (READ_ONCE(nlk->cb_running) &&
	    atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2) {
		ret = netlink_dump(sk, false);
    ...
}

The sk_rmem_alloc value read there still includes the charge of a
notification that was dequeued but is held through skb_get() in
do_one_broadcast(). That charge stays until consume_skb(skb) in
netlink_broadcast_filtered().

Suppose that notification's truesize is between rcvbuf/2 and rcvbuf:

  1. The dump starts normally under the new check.
  2. The reader consumes the first dump skb.
  3. The gate sees the stale charge above rcvbuf/2 and does not
     continue the dump.
  4. The next recvmsg() blocks on an empty queue.
  5. netlink_skb_destructor()->sock_rfree() later uncharges, but neither
     wakes the reader nor calls netlink_dump().

A dump that needs several skbs, such as a scan dump, then stalls until
some unrelated message arrives. Could this be handled together with the
start-of-dump case? The gate and the destructor are not changed here, and
they behaved the same way before ae8f160e7eb2.

>  		atomic_sub(skb->truesize, &sk->sk_rmem_alloc);
>  		goto errout_skb;
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-netlink-dump-livelock-v2-0-c4bb05352a33%40gmail.com