| From 876886f822fabe334d854628a514f423a00d0b91 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Tue, 1 Apr 2025 11:44:43 -0700 |
| Subject: udp: Fix memory accounting leak. |
| |
| From: Kuniyuki Iwashima <kuniyu@amazon.com> |
| |
| [ Upstream commit df207de9d9e7a4d92f8567e2c539d9c8c12fd99d ] |
| |
| Matt Dowling reported a weird UDP memory usage issue. |
| |
| Under normal operation, the UDP memory usage reported in /proc/net/sockstat |
| remains close to zero. However, it occasionally spiked to 524,288 pages |
| and never dropped. Moreover, the value doubled when the application was |
| terminated. Finally, it caused intermittent packet drops. |
| |
| We can reproduce the issue with the script below [0]: |
| |
| 1. /proc/net/sockstat reports 0 pages |
| |
| # cat /proc/net/sockstat | grep UDP: |
| UDP: inuse 1 mem 0 |
| |
| 2. Run the script till the report reaches 524,288 |
| |
| # python3 test.py & sleep 5 |
| # cat /proc/net/sockstat | grep UDP: |
| UDP: inuse 3 mem 524288 <-- (INT_MAX + 1) >> PAGE_SHIFT |
| |
| 3. Kill the socket and confirm the number never drops |
| |
| # pkill python3 && sleep 5 |
| # cat /proc/net/sockstat | grep UDP: |
| UDP: inuse 1 mem 524288 |
| |
| 4. (necessary since v6.0) Trigger proto_memory_pcpu_drain() |
| |
| # python3 test.py & sleep 1 && pkill python3 |
| |
| 5. The number doubles |
| |
| # cat /proc/net/sockstat | grep UDP: |
| UDP: inuse 1 mem 1048577 |
| |
| The application set INT_MAX to SO_RCVBUF, which triggered an integer |
| overflow in udp_rmem_release(). |
| |
| When a socket is close()d, udp_destruct_common() purges its receive |
| queue and sums up skb->truesize in the queue. This total is calculated |
| and stored in a local unsigned integer variable. |
| |
| The total size is then passed to udp_rmem_release() to adjust memory |
| accounting. However, because the function takes a signed integer |
| argument, the total size can wrap around, causing an overflow. |
| |
| Then, the released amount is calculated as follows: |
| |
| 1) Add size to sk->sk_forward_alloc. |
| 2) Round down sk->sk_forward_alloc to the nearest lower multiple of |
| PAGE_SIZE and assign it to amount. |
| 3) Subtract amount from sk->sk_forward_alloc. |
| 4) Pass amount >> PAGE_SHIFT to __sk_mem_reduce_allocated(). |
| |
| When the issue occurred, the total in udp_destruct_common() was 2147484480 |
| (INT_MAX + 833), which was cast to -2147482816 in udp_rmem_release(). |
| |
| At 1) sk->sk_forward_alloc is changed from 3264 to -2147479552, and |
| 2) sets -2147479552 to amount. 3) reverts the wraparound, so we don't |
| see a warning in inet_sock_destruct(). However, udp_memory_allocated |
| ends up doubling at 4). |
| |
| Since commit 3cd3399dd7a8 ("net: implement per-cpu reserves for |
| memory_allocated"), memory usage no longer doubles immediately after |
| a socket is close()d because __sk_mem_reduce_allocated() caches the |
| amount in udp_memory_per_cpu_fw_alloc. However, the next time a UDP |
| socket receives a packet, the subtraction takes effect, causing UDP |
| memory usage to double. |
| |
| This issue makes further memory allocation fail once the socket's |
| sk->sk_rmem_alloc exceeds net.ipv4.udp_rmem_min, resulting in packet |
| drops. |
| |
| To prevent this issue, let's use unsigned int for the calculation and |
| call sk_forward_alloc_add() only once for the small delta. |
| |
| Note that first_packet_length() also potentially has the same problem. |
| |
| [0]: |
| from socket import * |
| |
| SO_RCVBUFFORCE = 33 |
| INT_MAX = (2 ** 31) - 1 |
| |
| s = socket(AF_INET, SOCK_DGRAM) |
| s.bind(('', 0)) |
| s.setsockopt(SOL_SOCKET, SO_RCVBUFFORCE, INT_MAX) |
| |
| c = socket(AF_INET, SOCK_DGRAM) |
| c.connect(s.getsockname()) |
| |
| data = b'a' * 100 |
| |
| while True: |
| c.send(data) |
| |
| Fixes: f970bd9e3a06 ("udp: implement memory accounting helpers") |
| Reported-by: Matt Dowling <madowlin@amazon.com> |
| Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> |
| Reviewed-by: Willem de Bruijn <willemb@google.com> |
| Link: https://patch.msgid.link/20250401184501.67377-3-kuniyu@amazon.com |
| Signed-off-by: Jakub Kicinski <kuba@kernel.org> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| net/ipv4/udp.c | 16 +++++++--------- |
| 1 file changed, 7 insertions(+), 9 deletions(-) |
| |
| diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c |
| index b84d18fcd9e2c..dc91699ce0328 100644 |
| --- a/net/ipv4/udp.c |
| +++ b/net/ipv4/udp.c |
| @@ -1412,12 +1412,12 @@ static bool udp_skb_has_head_state(struct sk_buff *skb) |
| } |
| |
| /* fully reclaim rmem/fwd memory allocated for skb */ |
| -static void udp_rmem_release(struct sock *sk, int size, int partial, |
| - bool rx_queue_lock_held) |
| +static void udp_rmem_release(struct sock *sk, unsigned int size, |
| + int partial, bool rx_queue_lock_held) |
| { |
| struct udp_sock *up = udp_sk(sk); |
| struct sk_buff_head *sk_queue; |
| - int amt; |
| + unsigned int amt; |
| |
| if (likely(partial)) { |
| up->forward_deficit += size; |
| @@ -1437,10 +1437,8 @@ static void udp_rmem_release(struct sock *sk, int size, int partial, |
| if (!rx_queue_lock_held) |
| spin_lock(&sk_queue->lock); |
| |
| - |
| - sk_forward_alloc_add(sk, size); |
| - amt = (sk->sk_forward_alloc - partial) & ~(PAGE_SIZE - 1); |
| - sk_forward_alloc_add(sk, -amt); |
| + amt = (size + sk->sk_forward_alloc - partial) & ~(PAGE_SIZE - 1); |
| + sk_forward_alloc_add(sk, size - amt); |
| |
| if (amt) |
| __sk_mem_reduce_allocated(sk, amt >> PAGE_SHIFT); |
| @@ -1630,7 +1628,7 @@ EXPORT_SYMBOL_GPL(skb_consume_udp); |
| |
| static struct sk_buff *__first_packet_length(struct sock *sk, |
| struct sk_buff_head *rcvq, |
| - int *total) |
| + unsigned int *total) |
| { |
| struct sk_buff *skb; |
| |
| @@ -1663,8 +1661,8 @@ static int first_packet_length(struct sock *sk) |
| { |
| struct sk_buff_head *rcvq = &udp_sk(sk)->reader_queue; |
| struct sk_buff_head *sk_queue = &sk->sk_receive_queue; |
| + unsigned int total = 0; |
| struct sk_buff *skb; |
| - int total = 0; |
| int res; |
| |
| spin_lock_bh(&rcvq->lock); |
| -- |
| 2.39.5 |
| |