Fix netlink rcvbuf wraparound

For netlink sockets, when comparing allocated rmem memory with the rcvbuf limit, the comparison is done using signed values. This means that if rcvbuf is near INT_MAX, then sk->sk_rmem_alloc may become negative in the comparison with rcvbuf which will yield incorrect results.

This can be reproduced by using the program from SOCK_DIAG(7) with some slight modifications. First, setting sk->sk_rcvbuf to INT_MAX using SO_RCVBUFFORCE and then secondly running the "send_query()" in a loop while not calling "receive_responses()". In this case, the value of sk->sk_rmem_alloc will continuously wrap around and thus more memory is allocated than the sk->sk_rcvbuf limit. This will eventually fill all of memory leading to an out of memory condition with skbs filling up the slab.

Let's fix this in a similar manner to:
commit 5a465a0da13e ("udp: Fix multiple wraparounds of sk->sk_rmem_alloc.")

As noted in that fix, if there are multiple threads writing to a netlink socket it's possible to slightly exceed rcvbuf value. But as noted this avoids an expensive 'atomic_add_return()' for the common case.

From: Jason Baron <[email protected]>
Date: Wed, 18 Jun 2025 19:13:23 -0400

Quoted 15 lines

For netlink sockets, when comparing allocated rmem memory with the rcvbuf limit, the comparison is done using signed values. This means that if rcvbuf is near INT_MAX, then sk->sk_rmem_alloc may become negative in the comparison with rcvbuf which will yield incorrect results.

This can be reproduced by using the program from SOCK_DIAG(7) with some slight modifications. First, setting sk->sk_rcvbuf to INT_MAX using SO_RCVBUFFORCE and then secondly running the "send_query()" in a loop while not calling "receive_responses()". In this case, the value of sk->sk_rmem_alloc will continuously wrap around and thus more memory is allocated than the sk->sk_rcvbuf limit. This will eventually fill all of memory leading to an out of memory condition with skbs filling up the slab.

Quoted 7 lines

Let's fix this in a similar manner to:
commit 5a465a0da13e ("udp: Fix multiple wraparounds of sk->sk_rmem_alloc.")

As noted in that fix, if there are multiple threads writing to a netlink socket it's possible to slightly exceed rcvbuf value. But as noted this avoids an expensive 'atomic_add_return()' for the common case.

This was because UDP RX path is the fast path, but netlink isn't. Also, it's common for UDP that multiple packets for the same socket are processed concurrently, and 850cbaddb52d dropped lock_sock from the path.

Quoted 25 lines · On Wed, 18 Jun 2025 23:13:02 -0700 Kuniyuki Iwashima wrote:

From: Jason Baron <[email protected]>
Date: Wed, 18 Jun 2025 19:13:23 -0400

Quoted 22 lines

For netlink sockets, when comparing allocated rmem memory with the rcvbuf limit, the comparison is done using signed values. This means that if rcvbuf is near INT_MAX, then sk->sk_rmem_alloc may become negative in the comparison with rcvbuf which will yield incorrect results.

This can be reproduced by using the program from SOCK_DIAG(7) with some slight modifications. First, setting sk->sk_rcvbuf to INT_MAX using SO_RCVBUFFORCE and then secondly running the "send_query()" in a loop while not calling "receive_responses()". In this case, the value of sk->sk_rmem_alloc will continuously wrap around and thus more memory is allocated than the sk->sk_rcvbuf limit. This will eventually fill all of memory leading to an out of memory condition with skbs filling up the slab.

Let's fix this in a similar manner to:
commit 5a465a0da13e ("udp: Fix multiple wraparounds of sk->sk_rmem_alloc.")

As noted in that fix, if there are multiple threads writing to a netlink socket it's possible to slightly exceed rcvbuf value. But as noted this avoids an expensive 'atomic_add_return()' for the common case.

Quoted 4 lines

This was because UDP RX path is the fast path, but netlink isn't. Also, it's common for UDP that multiple packets for the same socket are processed concurrently, and 850cbaddb52d dropped lock_sock from the path.

To be clear -- are you saying we should fix this differently? Or perhaps that the problem doesn't exist? The change doesn't seem very intrusive..

!-------------------------------------------------------------------|
This Message Is From an External Sender
This message came from outside your organization.
|-------------------------------------------------------------------!

Quoted 26 lines · Jakub Kicinski

Quoted 25 lines · On Wed, 18 Jun 2025 23:13:02 -0700 Kuniyuki Iwashima wrote:

From: Jason Baron <[email protected]>
Date: Wed, 18 Jun 2025 19:13:23 -0400

Quoted 22 lines

For netlink sockets, when comparing allocated rmem memory with the rcvbuf limit, the comparison is done using signed values. This means that if rcvbuf is near INT_MAX, then sk->sk_rmem_alloc may become negative in the comparison with rcvbuf which will yield incorrect results.

This can be reproduced by using the program from SOCK_DIAG(7) with some slight modifications. First, setting sk->sk_rcvbuf to INT_MAX using SO_RCVBUFFORCE and then secondly running the "send_query()" in a loop while not calling "receive_responses()". In this case, the value of sk->sk_rmem_alloc will continuously wrap around and thus more memory is allocated than the sk->sk_rcvbuf limit. This will eventually fill all of memory leading to an out of memory condition with skbs filling up the slab.

Let's fix this in a similar manner to:
commit 5a465a0da13e ("udp: Fix multiple wraparounds of sk->sk_rmem_alloc.")

As noted in that fix, if there are multiple threads writing to a netlink socket it's possible to slightly exceed rcvbuf value. But as noted this avoids an expensive 'atomic_add_return()' for the common case.

Quoted 8 lines

Quoted 4 lines

This was because UDP RX path is the fast path, but netlink isn't. Also, it's common for UDP that multiple packets for the same socket are processed concurrently, and 850cbaddb52d dropped lock_sock from the path.

To be clear -- are you saying we should fix this differently? Or perhaps that the problem doesn't exist? The change doesn't seem very intrusive..

AFAICS the race is possible even with netlink as netlink_unicast() runs without the socket lock, too.

The point is that for UDP the scenario with multiple threads enqueuing a packet into the same socket is a critical path, optimizing for performances and allowing some memory accounting inaccuracy makes sense.

For netlink socket, that scenario looks a patological one and I think we should prefer accuracy instead of optimization.

Thanks,

Paolo

Quoted 3 lines

To be clear -- are you saying we should fix this differently? Or perhaps that the problem doesn't exist? The change doesn't seem very intrusive..

AFAICS the race is possible even with netlink as netlink_unicast() runs without the socket lock, too.

The point is that for UDP the scenario with multiple threads enqueuing a packet into the same socket is a critical path, optimizing for performances and allowing some memory accounting inaccuracy makes sense.

For netlink socket, that scenario looks a patological one and I think we should prefer accuracy instead of optimization.

Quoted 11 lines

Quoted 3 lines

To be clear -- are you saying we should fix this differently? Or perhaps that the problem doesn't exist? The change doesn't seem very intrusive..

AFAICS the race is possible even with netlink as netlink_unicast() runs without the socket lock, too.

The point is that for UDP the scenario with multiple threads enqueuing a packet into the same socket is a critical path, optimizing for performances and allowing some memory accounting inaccuracy makes sense.

Quoted 2 lines

For netlink socket, that scenario looks a patological one and I think we should prefer accuracy instead of optimization.

Could you ELI5 what you mean? Are you suggesting a lock around every sk_rmem write for netlink sockets?
If we think this is an attack vector the attacker can simply use a UDP socket instead. Or do you think it'd lead to simpler code?

Date: Tue, 24 Jun 2025 07:11:57 -0700
From: Jakub Kicinski <[email protected]>

Quoted 15 lines · On Tue, 24 Jun 2025 09:55:15 +0200 Paolo Abeni wrote:

Quoted 13 lines

Quoted 3 lines

To be clear -- are you saying we should fix this differently? Or perhaps that the problem doesn't exist? The change doesn't seem very intrusive..

AFAICS the race is possible even with netlink as netlink_unicast() runs without the socket lock, too.

The point is that for UDP the scenario with multiple threads enqueuing a packet into the same socket is a critical path, optimizing for performances and allowing some memory accounting inaccuracy makes sense.

For netlink socket, that scenario looks a patological one and I think we should prefer accuracy instead of optimization.

Quoted 4 lines

Could you ELI5 what you mean? Are you suggesting a lock around every sk_rmem write for netlink sockets?
If we think this is an attack vector the attacker can simply use a UDP socket instead. Or do you think it'd lead to simpler code?

I was wondering if atomic_add_return() is expensive for netlink, and if not, we could use it like below.

I'm also not sure we want to keep the allow-at-least-one-skb rule for netlink though, which comes from the first condition in __sock_queue_rcv_skb() for UDP in the past, IIRC.

Quoted 22 lines · On Tue, 24 Jun 2025 10:08:41 -0700 Kuniyuki Iwashima wrote:

Date: Tue, 24 Jun 2025 07:11:57 -0700
From: Jakub Kicinski <[email protected]>

Quoted 19 lines · On Tue, 24 Jun 2025 09:55:15 +0200 Paolo Abeni wrote:

Quoted 7 lines

Quoted 3 lines

To be clear -- are you saying we should fix this differently? Or perhaps that the problem doesn't exist? The change doesn't seem very intrusive..

AFAICS the race is possible even with netlink as netlink_unicast() runs without the socket lock, too.

Quoted 6 lines

The point is that for UDP the scenario with multiple threads enqueuing a packet into the same socket is a critical path, optimizing for performances and allowing some memory accounting inaccuracy makes sense.

For netlink socket, that scenario looks a patological one and I think we should prefer accuracy instead of optimization.

Could you ELI5 what you mean? Are you suggesting a lock around every sk_rmem write for netlink sockets?
If we think this is an attack vector the attacker can simply use a UDP socket instead. Or do you think it'd lead to simpler code?

Quoted 2 lines

I was wondering if atomic_add_return() is expensive for netlink, and if not, we could use it like below.

Ah, got it. That does look simpler.

nit: Please don't hide the atomic_add_return() in local variable init, as it need validation and error handling.