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

[Jhonata Poma-Hansen]

On Thu, 30 Oct 2026, Sashiko wrote:
> [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...

This is real, but it is not new: both the previous check and the one
before ae8f160e7eb2 refuse the skb when the stale charge alone is at or
above sk_rcvbuf. It needs a single notification with a truesize of at
least sk_rcvbuf, so only very small receive buffers. The patch fixes
the common case where the stale charge is below sk_rcvbuf; the commit
message could have been narrower about that.

Never failing the first skb on rcvbuf would let a reader that does not
read queue one skb per dump request without bound, which is what
ae8f160e7eb2 closed.

> [Medium] This problem wasn't introduced by this patch, but it has the
> same trigger the commit describes.

Right, the gate in netlink_recvmsg() is unchanged by this patch.

Thanks,
Jhonata

Best regards,
Jhonata Poma-Hansen

🇩🇰 +45 7143 7020 🇮🇹 +39 3205740789




Il giorno ven 30 ott 2026 alle ore 03:05
<netdev-bot+sashiko@xxxxxxxxxx> ha scritto:
>
> 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