[PATCH net v4 0/2] tcp: clamp return value of tcp_wmem_schedule() to non-negative

[PATCH net v4 0/2] tcp: clamp return value of tcp_wmem_schedule() to non-negative

[Octavian Purdila]

This series fixes a bug discovered in production in tcp_wmem_schedule()
where a negative sk->sk_forward_alloc under TCP memory pressure causes
tcp_wmem_schedule() to return a negative copy length when
sk->sk_wmem_queued already reaches or exceeds sysctl_tcp_wmem[0].

sk->sk_forward_alloc can transiently become negative when tcp_collapse()
re-packs partially read or high payload-to-truesize SKBs in the receive
queue and charges newly allocated SKBs via sk_mem_charge() before
freeing the original SKBs. When a subsequent send hits memory pressure
in sk_wmem_schedule() with sk->sk_wmem_queued >= sysctl_tcp_wmem[0],
tcp_wmem_schedule() returns min(copy, sk->sk_forward_alloc) < 0.
Callers such as tcp_sendmsg_locked() then pass that negative copy length
to skb_copy_to_page_nocache() -> copy_from_iter_full() ->
check_copy_size(), triggering WARN_ON_ONCE(bytes > INT_MAX) in
production and returning -EFAULT instead of waiting for memory or
returning -EAGAIN.

Patch 1 clamps the return value of tcp_wmem_schedule() to [0, copy] so
a negative sk->sk_forward_alloc is treated as 0 available bytes.
Patch 2 adds a packetdrill selftest that reproduces the negative
sk->sk_forward_alloc condition via tcp_collapse() under memory pressure
and verifies that a non-blocking write fails with EAGAIN rather than
EFAULT.

Changes in v4:
- Add --define=REQUIRES_MEMCG="1" support in ksft_runner.sh to skip the
  test when the cgroup2 memory controller is unavailable and clean up
  pkt_* cgroups and cgroup.subtree_control via an EXIT trap.
- Pin packetdrill to CPU 0, write 0 to memory.high twice to guarantee
  local per-CPU memcg stock is drained synchronously, and set
  memory.swap.max=0 in the test cgroup.
- Fix checkpatch warnings (duplicate Signed-off-by trailer and comment
  line length) and preserve the "net" target prefix on all patches.

Changes in v3:
- Fix packetdrill test failure in netdev CI (CONFIG_MAX_SKB_FRAGS=17):
  adjust segment sizes (35 KB) and buffer limits so the receive SKB
  truesize is identical across CONFIG_MAX_SKB_FRAGS=17 and 45, and
  tcp_collapse() deterministically drives sk_forward_alloc below -16 KB
  without invoking the memcg OOM killer.

Changes in v2:
- Include production warning trace in Patch 1 commit message.
- Skip the packetdrill test when PAGE_SIZE != 4096 via
  --define=REQUIRES_PAGESIZE="4096" in ksft_runner.sh.
- Collect Reviewed-by tags from Kuniyuki Iwashima and Eric Dumazet.

Octavian Purdila (2):
  tcp: clamp return value of tcp_wmem_schedule() to non-negative
  selftests: net: packetdrill: add test for negative sk_forward_alloc in
    tcp_wmem_schedule()

 net/ipv4/tcp.c                                |   2 +-
 .../testing/selftests/net/packetdrill/config  |   2 +
 .../selftests/net/packetdrill/ksft_runner.sh  |  28 +++++
 .../tcp_wmem_schedule_negative_fwd_alloc.pkt  | 116 ++++++++++++++++++
 4 files changed, 147 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt

-- 
2.56.0.257.gf7497c7d27-goog

[PATCH net v4 1/2] tcp: clamp return value of tcp_wmem_schedule() to non-negative

[Octavian Purdila]

When sk_wmem_schedule(sk, copy) fails under TCP memory pressure and
sk->sk_wmem_queued already reaches or exceeds sysctl_tcp_wmem[0],
tcp_wmem_schedule() returns min(copy, sk->sk_forward_alloc).

However, sk->sk_forward_alloc is a signed int that can transiently
become negative when tcp_collapse() re-packs partially read or high
payload-to-truesize SKBs in the receive queue and charges newly
allocated SKBs via sk_mem_charge() before freeing the original SKB.

If tcp_wmem_schedule() returns a negative value, tcp_sendmsg_locked()
passes that negative copy length to skb_copy_to_page_nocache() ->
copy_from_iter_full() -> check_copy_size(), where it is converted to an
unsigned size_t > INT_MAX, triggering a WARN_ON_ONCE(bytes > INT_MAX)
in production and returning -EFAULT instead of waiting for memory or
returning -EAGAIN:

  WARNING: CPU: 1 PID: 939331 at include/linux/thread_info.h:249 tcp_sendmsg+0x10c1/0x1530
  Call Trace:
   <TASK>
   __sys_sendmsg+0x292/0x490
   do_syscall_64+0x88/0x340
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

Clamp the return value of tcp_wmem_schedule() to [0, copy] so callers
treat negative sk_forward_alloc as 0 available bytes.

Fixes: 849b425cd091 ("tcp: fix possible freeze in tx path under memory pressure")
Assisted-by: LLM
Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
Reviewed-by: Eric Dumazet <edumazet@kernel.org>
Signed-off-by: Octavian Purdila <tavip@google.com>
---
 net/ipv4/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 87ef6d5cbfeba..0e062fcd94a6c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1037,7 +1037,7 @@ int tcp_wmem_schedule(struct sock *sk, int copy)
 	left = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_wmem[0]) - sk->sk_wmem_queued;
 	if (left > 0)
 		sk_forced_mem_schedule(sk, min(left, copy));
-	return min(copy, sk->sk_forward_alloc);
+	return clamp(sk->sk_forward_alloc, 0, copy);
 }
 
 void tcp_free_fastopen_req(struct tcp_sock *tp)
-- 
2.56.0.257.gf7497c7d27-goog

[PATCH net v4 2/2] selftests: net: packetdrill: add test for negative sk_forward_alloc in tcp_wmem_schedule()

[Octavian Purdila]

Add a packetdrill test that drives sk->sk_forward_alloc negative via
tcp_collapse() on the receive queue while sk->sk_wmem_queued exceeds
sysctl_tcp_wmem[0], and verifies that a subsequent non-blocking write()
under TCP memory pressure fails with EAGAIN rather than returning a
negative copy length from tcp_wmem_schedule() and failing with EFAULT.

Assisted-by: LLM
Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
Signed-off-by: Octavian Purdila <tavip@google.com>
---
 .../testing/selftests/net/packetdrill/config  |   2 +
 .../selftests/net/packetdrill/ksft_runner.sh  |  28 +++++
 .../tcp_wmem_schedule_negative_fwd_alloc.pkt  | 116 ++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt

diff --git a/tools/testing/selftests/net/packetdrill/config b/tools/testing/selftests/net/packetdrill/config
index b7df8bf309200..a14a704dcbd49 100644
--- a/tools/testing/selftests/net/packetdrill/config
+++ b/tools/testing/selftests/net/packetdrill/config
@@ -1,6 +1,8 @@
+CONFIG_CGROUPS=y
 CONFIG_HZ=1000
 CONFIG_HZ_1000=y
 CONFIG_IPV6=y
+CONFIG_MEMCG=y
 CONFIG_NET_NS=y
 CONFIG_NET_SCH_FIFO=y
 CONFIG_NET_SCH_FQ=y
diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh
index 0a97d5ae34699..ba62f3840eb57 100755
--- a/tools/testing/selftests/net/packetdrill/ksft_runner.sh
+++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh
@@ -52,6 +52,34 @@ if [[ -n "${KSFT_MACHINE_SLOW}" ]]; then
 	failfunc=ktap_test_xfail
 fi
 
+pagesize=$(grep -E '^--define=REQUIRES_PAGESIZE=' $script | cut -d '=' -f 3 | tr -d '"')
+if [[ -n "$pagesize" && "$(getconf PAGESIZE)" != "$pagesize" ]]; then
+	ktap_skip_all "requires ${pagesize}B PAGE_SIZE"
+	exit "$KSFT_SKIP"
+fi
+
+if grep -q -E '^--define=REQUIRES_MEMCG=' $script; then
+	cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
+	memcg_was_enabled=0
+	if [[ -n "$cgroot" ]] &&
+	   grep -qw memory "${cgroot}/cgroup.subtree_control" 2>/dev/null; then
+		memcg_was_enabled=1
+	fi
+	if [[ -z "$cgroot" ]] ||
+	   ! echo +memory > "${cgroot}/cgroup.subtree_control" 2>/dev/null; then
+		ktap_skip_all "requires cgroup2 memory controller"
+		exit "$KSFT_SKIP"
+	fi
+	cleanup_memcg() {
+		rmdir "${cgroot}"/pkt_* 2>/dev/null || true
+		if [[ "$memcg_was_enabled" -eq 0 ]]; then
+			echo -memory > "${cgroot}/cgroup.subtree_control" \
+				2>/dev/null || true
+		fi
+	}
+	trap cleanup_memcg EXIT
+fi
+
 ip_versions=$(grep -E '^--ip_version=' $script | cut -d '=' -f 2)
 if [[ -z $ip_versions ]]; then
 	ip_versions="ipv4 ipv6 ipv4-mapped-ipv6"
diff --git a/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt b/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt
new file mode 100644
index 0000000000000..e4d8d13144f0a
--- /dev/null
+++ b/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+// Test that tcp_wmem_schedule() clamps negative sk->sk_forward_alloc to 0
+// when sk_wmem_schedule() fails under memory pressure and
+// sk->sk_wmem_queued >= sysctl_tcp_wmem[0], returning EAGAIN rather than
+// EFAULT.
+//
+// Buffer and segment sizes are tuned for 4 KB pages and standard
+// CONFIG_MAX_SKB_FRAGS values (17 to 45).
+
+--define=REQUIRES_PAGESIZE="4096"
+--define=REQUIRES_MEMCG="1"
+--mtu=65535
+--tolerance_usecs=100000
+
+`set -e
+./defaults.sh
+ip link set dev tun0 mtu 65535
+sysctl -q net.ipv4.tcp_rmem="4096 131072 6291456"
+sysctl -q net.ipv4.tcp_wmem="4096 16384 4194304"
+taskset -cp 0 $PPID >/dev/null 2>&1 || true
+cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
+[ -n "$cgroot" ]
+echo +memory > "${cgroot}/cgroup.subtree_control"
+rmdir "${cgroot}/pkt_${PPID}" 2>/dev/null || true
+mkdir "${cgroot}/pkt_${PPID}"
+echo 0 > "${cgroot}/pkt_${PPID}/memory.swap.max" 2>/dev/null || true
+echo $PPID > "${cgroot}/pkt_${PPID}/cgroup.procs"
+`
+
+// Create listening socket in pkt_${PPID} memcg and establish connection
+0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
++0.000 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
++0.000 bind(3, ..., ...) = 0
++0.000 listen(3, 1) = 0
+
++0.000 < S 0:0(0) win 65535 <mss 35000,nop,wscale 7>
++0.000 > S. 0:0(0) ack 1 <...>
++0.000 < . 1:1(0) ack 1 win 512
++0.000 accept(3, ..., ...) = 4
+
+// Move packetdrill back to its original cgroup so subsequent packetdrill
+// allocations are not charged to pkt_${PPID}.
++0.000 `cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
+runner_pid=$(awk '/^PPid:/ {print $2}' /proc/$PPID/status)
+orig_cg=$(sed -n 's/^0:://p' /proc/$runner_pid/cgroup)
+echo $PPID > "${cgroot}${orig_cg}/cgroup.procs"`
+
++0.000 setsockopt(4, SOL_SOCKET, SO_SNDBUF, [200000], 4) = 0
++0.000 setsockopt(4, SOL_SOCKET, SO_RCVBUF, [112500], 4) = 0
+
+// 1. Warm up rcv_ssthresh to full window
++0.000 < P. 1:35001(35000) ack 1 win 512
++0.000 > . 1:1(0) ack 35001
++0.000 read(4, ..., 35000) = 35000
+
++0.000 < P. 35001:70001(35000) ack 1 win 512
++0.000 > . 1:1(0) ack 70001
++0.000 read(4, ..., 35000) = 35000
+
++0.000 < P. 70001:105001(35000) ack 1 win 512
++0.000 > . 1:1(0) ack 105001
++0.000 read(4, ..., 35000) = 35000
+
++0.000 < P. 105001:140001(35000) ack 1 win 512
++0.000 > . 1:1(0) ack 140001
++0.000 read(4, ..., 35000) = 35000
+
+// 2. Queue 79 KB on TX so sk_wmem_queued >= sysctl_tcp_wmem[0], while leaving
+//    both tail SKB space (26 KB) and sk_page_frag space (19.3 KB) >= 16 KB.
++0.000 write(4, ..., 79000) = 79000
+
+// 3a. Inject 6 high-density 35 KB segments (210 KB payload, 222.7 KB truesize
+//     across 8-page order-3 frags on both CONFIG_MAX_SKB_FRAGS=17 and 45) and
+//     read 100 bytes from the head SKB so before(skb->seq, tp->copied_seq)
+//     holds.
++0.000 < P. 140001:175001(35000) ack 1 win 512
++0.000 < P. 175001:210001(35000) ack 1 win 512
++0.000 read(4, ..., 100) = 100
+
++0.000 < P. 210001:245001(35000) ack 1 win 512
++0.000 < P. 245001:280001(35000) ack 1 win 512
++0.000 < P. 280001:315001(35000) ack 1 win 512
++0.000 < P. 315001:350001(35000) ack 1 win 512
+
+// 3b. Inject three smaller segments into the remaining advertised rcv_wnd.
+//     Their higher truesize pushes sk_rmem_alloc > sk_rcvbuf (225000) and
+//     exhausts sk_forward_alloc, triggering tcp_prune_queue() -> tcp_collapse()
+//     on the 3rd segment and driving sk->sk_forward_alloc <= -16.4 KB.
++0.000 < P. 350001:350801(800) ack 1 win 512
++0.000 < P. 350801:351601(800) ack 1 win 512
++0.000 < P. 351601:351901(300) ack 1 win 512
+
+// 4. Drain per-CPU memcg stock via memory.high=0, cap memory.max=0 (O_NONBLOCK
+//    avoids synchronous reclaim/OOM), and expect -EAGAIN on a 16 KB non-blocking
+//    write. Because copy (16 KB) minus sk_forward_alloc (<= -16.4 KB) exceeds
+//    PAGE_ALLOC_COSTLY_ORDER (32 KB), try_charge_memcg() fails without invoking
+//    the memcg OOM killer.
++0.010 `cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
+echo 0 > "${cgroot}/pkt_${PPID}/memory.high"
+echo 0 > "${cgroot}/pkt_${PPID}/memory.high"
+printf 0 | dd of="${cgroot}/pkt_${PPID}/memory.max" oflag=nonblock status=none`
++0.000 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
++0.000 write(4, ..., 16384) = -1 EAGAIN (Resource temporarily unavailable)
+
++0.000 close(4) = 0
++0.000 close(3) = 0
+
+// Untimed cleanup command executed by packetdrill on both pass and early
+// failure.
+`cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
+[ -n "$cgroot" ] || exit 0
+runner_pid=$(awk '/^PPid:/ {print $2}' /proc/$PPID/status)
+orig_cg=$(sed -n 's/^0:://p' /proc/$runner_pid/cgroup)
+echo $PPID > "${cgroot}${orig_cg}/cgroup.procs" 2>/dev/null || true
+echo $$ > "${cgroot}${orig_cg}/cgroup.procs" 2>/dev/null || true
+rmdir "${cgroot}/pkt_${PPID}"`
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH net v4 1/2] tcp: clamp return value of tcp_wmem_schedule() to non-negative

[Eric Dumazet]

Le lun. 26 oct. 2026 à 04:22, Octavian Purdila <tavip@google.com> a écrit :
>
> When sk_wmem_schedule(sk, copy) fails under TCP memory pressure and
> sk->sk_wmem_queued already reaches or exceeds sysctl_tcp_wmem[0],
> tcp_wmem_schedule() returns min(copy, sk->sk_forward_alloc).
>
> However, sk->sk_forward_alloc is a signed int that can transiently
> become negative when tcp_collapse() re-packs partially read or high
> payload-to-truesize SKBs in the receive queue and charges newly
> allocated SKBs via sk_mem_charge() before freeing the original SKB.
>
> If tcp_wmem_schedule() returns a negative value, tcp_sendmsg_locked()
> passes that negative copy length to skb_copy_to_page_nocache() ->
> copy_from_iter_full() -> check_copy_size(), where it is converted to an
> unsigned size_t > INT_MAX, triggering a WARN_ON_ONCE(bytes > INT_MAX)
> in production and returning -EFAULT instead of waiting for memory or
> returning -EAGAIN:
>
>   WARNING: CPU: 1 PID: 939331 at include/linux/thread_info.h:249 tcp_sendmsg+0x10c1/0x1530
>   Call Trace:
>    <TASK>
>    __sys_sendmsg+0x292/0x490
>    do_syscall_64+0x88/0x340
>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Clamp the return value of tcp_wmem_schedule() to [0, copy] so callers
> treat negative sk_forward_alloc as 0 available bytes.
>
> Fixes: 849b425cd091 ("tcp: fix possible freeze in tx path under memory pressure")
> Assisted-by: LLM
> Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
> Reviewed-by: Eric Dumazet <edumazet@kernel.org>
> Signed-off-by: Octavian Purdila <tavip@google.com>
> ---
>  net/ipv4/tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 87ef6d5cbfeba..0e062fcd94a6c 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1037,7 +1037,7 @@ int tcp_wmem_schedule(struct sock *sk, int copy)
>         left = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_wmem[0]) - sk->sk_wmem_queued;
>         if (left > 0)
>                 sk_forced_mem_schedule(sk, min(left, copy));
> -       return min(copy, sk->sk_forward_alloc);
> +       return clamp(sk->sk_forward_alloc, 0, copy);
>  }
>
>  void tcp_free_fastopen_req(struct tcp_sock *tp)

I do wonder if the following fix could be better (no change in the fast path)

Before a complete fix in tcp_collapse() (in net-next)

    tcp: do not let tcp_collapse() drive sk_forward_alloc negative

    tcp_collapse() charges each newly allocated skb with skb_set_owner_r()
    before the skbs it replaces are freed, without scheduling memory first.

    Since sk_mem_uncharge() reclaims eagerly, sk->sk_forward_alloc has no
    cushion and can become negative. This can be durable when the collapsed
    skbs had a high payload/truesize ratio or were partially read, because
    the new skbs can have a bigger total truesize than the old ones.

    A negative sk_forward_alloc then confuses tcp_wmem_schedule(), which
    can return a negative length to tcp_sendmsg_locked(), triggering
    a WARN_ON_ONCE(bytes > INT_MAX) in check_copy_size().

    Call sk_forced_mem_schedule() before charging nskb.

    Fixes: 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 1ddce9c4b7e8cf91bfd4f5361abec3925fe25046..e879bdfddf1abe2ced80b12ea8a40c1f7c958708
100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5847,6 +5847,10 @@ tcp_collapse(struct sock *sk, struct
sk_buff_head *list, struct rb_root *root,
                        __skb_queue_before(list, skb, nskb);
                else
                        __skb_queue_tail(&tmp, nskb); /* defer rbtree
insertion */
+               /* nskb is charged before the skbs it replaces are freed.
+                * Make sure sk->sk_forward_alloc can not become negative.
+                */
+               sk_forced_mem_schedule(sk, nskb->truesize);
                skb_set_owner_r(nskb, sk);
                mptcp_skb_ext_move(nskb, skb);

Re: [PATCH net v4 1/2] tcp: clamp return value of tcp_wmem_schedule() to non-negative

[Kuniyuki Iwashima]

On Mon, 26 Oct 2026 13:06:56 +0100, Eric Dumazet wrote:
> Le lun. 26 oct. 2026 à 04:22, Octavian Purdila <tavip@google.com> a écrit :
> >
> > When sk_wmem_schedule(sk, copy) fails under TCP memory pressure and
> > sk->sk_wmem_queued already reaches or exceeds sysctl_tcp_wmem[0],
> > tcp_wmem_schedule() returns min(copy, sk->sk_forward_alloc).
> >
> > However, sk->sk_forward_alloc is a signed int that can transiently
> > become negative when tcp_collapse() re-packs partially read or high
> > payload-to-truesize SKBs in the receive queue and charges newly
> > allocated SKBs via sk_mem_charge() before freeing the original SKB.
> >
> > If tcp_wmem_schedule() returns a negative value, tcp_sendmsg_locked()
> > passes that negative copy length to skb_copy_to_page_nocache() ->
> > copy_from_iter_full() -> check_copy_size(), where it is converted to an
> > unsigned size_t > INT_MAX, triggering a WARN_ON_ONCE(bytes > INT_MAX)
> > in production and returning -EFAULT instead of waiting for memory or
> > returning -EAGAIN:
> >
> >   WARNING: CPU: 1 PID: 939331 at include/linux/thread_info.h:249 tcp_sendmsg+0x10c1/0x1530
> >   Call Trace:
> >    <TASK>
> >    __sys_sendmsg+0x292/0x490
> >    do_syscall_64+0x88/0x340
> >    entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >
> > Clamp the return value of tcp_wmem_schedule() to [0, copy] so callers
> > treat negative sk_forward_alloc as 0 available bytes.
> >
> > Fixes: 849b425cd091 ("tcp: fix possible freeze in tx path under memory pressure")
> > Assisted-by: LLM
> > Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
> > Reviewed-by: Eric Dumazet <edumazet@kernel.org>
> > Signed-off-by: Octavian Purdila <tavip@google.com>
> > ---
> >  net/ipv4/tcp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 87ef6d5cbfeba..0e062fcd94a6c 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1037,7 +1037,7 @@ int tcp_wmem_schedule(struct sock *sk, int copy)
> >         left = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_wmem[0]) - sk->sk_wmem_queued;
> >         if (left > 0)
> >                 sk_forced_mem_schedule(sk, min(left, copy));
> > -       return min(copy, sk->sk_forward_alloc);
> > +       return clamp(sk->sk_forward_alloc, 0, copy);
> >  }
> >
> >  void tcp_free_fastopen_req(struct tcp_sock *tp)
> 
> I do wonder if the following fix could be better (no change in the fast path)

Exactly, this looks better too, we can avoid negative
sk_forward_alloc and clamp().

The packetdrill will need to be adjusted as it triggers cgroup
OOM with this approach, or we could defer the test to net-next too.


> 
> Before a complete fix in tcp_collapse() (in net-next)
> 
>     tcp: do not let tcp_collapse() drive sk_forward_alloc negative
> 
>     tcp_collapse() charges each newly allocated skb with skb_set_owner_r()
>     before the skbs it replaces are freed, without scheduling memory first.
> 
>     Since sk_mem_uncharge() reclaims eagerly, sk->sk_forward_alloc has no
>     cushion and can become negative. This can be durable when the collapsed
>     skbs had a high payload/truesize ratio or were partially read, because
>     the new skbs can have a bigger total truesize than the old ones.
> 
>     A negative sk_forward_alloc then confuses tcp_wmem_schedule(), which
>     can return a negative length to tcp_sendmsg_locked(), triggering
>     a WARN_ON_ONCE(bytes > INT_MAX) in check_copy_size().
> 
>     Call sk_forced_mem_schedule() before charging nskb.
> 
>     Fixes: 4890b686f408 ("net: keep sk->sk_forward_alloc as small as possible")
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 1ddce9c4b7e8cf91bfd4f5361abec3925fe25046..e879bdfddf1abe2ced80b12ea8a40c1f7c958708
> 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5847,6 +5847,10 @@ tcp_collapse(struct sock *sk, struct
> sk_buff_head *list, struct rb_root *root,
>                         __skb_queue_before(list, skb, nskb);
>                 else
>                         __skb_queue_tail(&tmp, nskb); /* defer rbtree
> insertion */
> +               /* nskb is charged before the skbs it replaces are freed.
> +                * Make sure sk->sk_forward_alloc can not become negative.
> +                */
> +               sk_forced_mem_schedule(sk, nskb->truesize);
>                 skb_set_owner_r(nskb, sk);
>                 mptcp_skb_ext_move(nskb, skb);

Re: [PATCH net v4 1/2] tcp: clamp return value of tcp_wmem_schedule() to non-negative

[Eric Dumazet]

Le mar. 27 oct. 2026 à 03:28, Kuniyuki Iwashima <kuniyu@google.com> a écrit :
>
> On Mon, 26 Oct 2026 13:06:56 +0100, Eric Dumazet wrote:
> > Le lun. 26 oct. 2026 à 04:22, Octavian Purdila <tavip@google.com> a écrit :
> > >
> > > When sk_wmem_schedule(sk, copy) fails under TCP memory pressure and
> > > sk->sk_wmem_queued already reaches or exceeds sysctl_tcp_wmem[0],
> > > tcp_wmem_schedule() returns min(copy, sk->sk_forward_alloc).
> > >
> > > However, sk->sk_forward_alloc is a signed int that can transiently
> > > become negative when tcp_collapse() re-packs partially read or high
> > > payload-to-truesize SKBs in the receive queue and charges newly
> > > allocated SKBs via sk_mem_charge() before freeing the original SKB.
> > >
> > > If tcp_wmem_schedule() returns a negative value, tcp_sendmsg_locked()
> > > passes that negative copy length to skb_copy_to_page_nocache() ->
> > > copy_from_iter_full() -> check_copy_size(), where it is converted to an
> > > unsigned size_t > INT_MAX, triggering a WARN_ON_ONCE(bytes > INT_MAX)
> > > in production and returning -EFAULT instead of waiting for memory or
> > > returning -EAGAIN:
> > >
> > >   WARNING: CPU: 1 PID: 939331 at include/linux/thread_info.h:249 tcp_sendmsg+0x10c1/0x1530
> > >   Call Trace:
> > >    <TASK>
> > >    __sys_sendmsg+0x292/0x490
> > >    do_syscall_64+0x88/0x340
> > >    entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > >
> > > Clamp the return value of tcp_wmem_schedule() to [0, copy] so callers
> > > treat negative sk_forward_alloc as 0 available bytes.
> > >
> > > Fixes: 849b425cd091 ("tcp: fix possible freeze in tx path under memory pressure")
> > > Assisted-by: LLM
> > > Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
> > > Reviewed-by: Eric Dumazet <edumazet@kernel.org>
> > > Signed-off-by: Octavian Purdila <tavip@google.com>
> > > ---
> > >  net/ipv4/tcp.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index 87ef6d5cbfeba..0e062fcd94a6c 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -1037,7 +1037,7 @@ int tcp_wmem_schedule(struct sock *sk, int copy)
> > >         left = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_wmem[0]) - sk->sk_wmem_queued;
> > >         if (left > 0)
> > >                 sk_forced_mem_schedule(sk, min(left, copy));
> > > -       return min(copy, sk->sk_forward_alloc);
> > > +       return clamp(sk->sk_forward_alloc, 0, copy);
> > >  }
> > >
> > >  void tcp_free_fastopen_req(struct tcp_sock *tp)
> >
> > I do wonder if the following fix could be better (no change in the fast path)
>
> Exactly, this looks better too, we can avoid negative
> sk_forward_alloc and clamp().
>
> The packetdrill will need to be adjusted as it triggers cgroup
> OOM with this approach, or we could defer the test to net-next too.
>

Let's defer the test. We also have an upcoming fix for tcp_collapse() anyway.

Octavian, could you resend a v5 (sorry for that) ?

Thanks!

pw-bot: cr

Re: [PATCH net v4 1/2] tcp: clamp return value of tcp_wmem_schedule() to non-negative

[Octavian Purdila]

On Wed, Oct 28, 2026 at 8:20 AM Eric Dumazet <edumazet@kernel.org> wrote:
>
> Le mar. 27 oct. 2026 à 03:28, Kuniyuki Iwashima <kuniyu@google.com> a écrit :
> >
> > On Mon, 26 Oct 2026 13:06:56 +0100, Eric Dumazet wrote:
> > > Le lun. 26 oct. 2026 à 04:22, Octavian Purdila <tavip@google.com> a écrit :
> > > >
> > > > When sk_wmem_schedule(sk, copy) fails under TCP memory pressure and
> > > > sk->sk_wmem_queued already reaches or exceeds sysctl_tcp_wmem[0],
> > > > tcp_wmem_schedule() returns min(copy, sk->sk_forward_alloc).
> > > >
> > > > However, sk->sk_forward_alloc is a signed int that can transiently
> > > > become negative when tcp_collapse() re-packs partially read or high
> > > > payload-to-truesize SKBs in the receive queue and charges newly
> > > > allocated SKBs via sk_mem_charge() before freeing the original SKB.
> > > >
> > > > If tcp_wmem_schedule() returns a negative value, tcp_sendmsg_locked()
> > > > passes that negative copy length to skb_copy_to_page_nocache() ->
> > > > copy_from_iter_full() -> check_copy_size(), where it is converted to an
> > > > unsigned size_t > INT_MAX, triggering a WARN_ON_ONCE(bytes > INT_MAX)
> > > > in production and returning -EFAULT instead of waiting for memory or
> > > > returning -EAGAIN:
> > > >
> > > >   WARNING: CPU: 1 PID: 939331 at include/linux/thread_info.h:249 tcp_sendmsg+0x10c1/0x1530
> > > >   Call Trace:
> > > >    <TASK>
> > > >    __sys_sendmsg+0x292/0x490
> > > >    do_syscall_64+0x88/0x340
> > > >    entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > > >
> > > > Clamp the return value of tcp_wmem_schedule() to [0, copy] so callers
> > > > treat negative sk_forward_alloc as 0 available bytes.
> > > >
> > > > Fixes: 849b425cd091 ("tcp: fix possible freeze in tx path under memory pressure")
> > > > Assisted-by: LLM
> > > > Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
> > > > Reviewed-by: Eric Dumazet <edumazet@kernel.org>
> > > > Signed-off-by: Octavian Purdila <tavip@google.com>
> > > > ---
> > > >  net/ipv4/tcp.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > index 87ef6d5cbfeba..0e062fcd94a6c 100644
> > > > --- a/net/ipv4/tcp.c
> > > > +++ b/net/ipv4/tcp.c
> > > > @@ -1037,7 +1037,7 @@ int tcp_wmem_schedule(struct sock *sk, int copy)
> > > >         left = READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_wmem[0]) - sk->sk_wmem_queued;
> > > >         if (left > 0)
> > > >                 sk_forced_mem_schedule(sk, min(left, copy));
> > > > -       return min(copy, sk->sk_forward_alloc);
> > > > +       return clamp(sk->sk_forward_alloc, 0, copy);
> > > >  }
> > > >
> > > >  void tcp_free_fastopen_req(struct tcp_sock *tp)
> > >
> > > I do wonder if the following fix could be better (no change in the fast path)
> >
> > Exactly, this looks better too, we can avoid negative
> > sk_forward_alloc and clamp().
> >
> > The packetdrill will need to be adjusted as it triggers cgroup
> > OOM with this approach, or we could defer the test to net-next too.
> >
>
> Let's defer the test. We also have an upcoming fix for tcp_collapse() anyway.
>
> Octavian, could you resend a v5 (sorry for that) ?
>

OK, no worries. I'll post your version as v5.

Re: [PATCH net v4 2/2] selftests: net: packetdrill: add test for negative sk_forward_alloc in tcp_wmem_schedule()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 0

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] The REQUIRES_MEMCG check in ksft_runner.sh (lines 61-72) only
  confirms that `+memory` can be written to the cgroup2 root…
- [Medium] cleanup_memcg() in
  tools/testing/selftests/net/packetdrill/ksft_runner.sh (lines 73-80)…
- [Medium] Step 4 of tcp_wmem_schedule_negative_fwd_alloc.pkt (lines
  98-101) depends on the two `echo 0 > memory.high` writes having…

--

selftests: net: packetdrill: add test for negative sk_forward_alloc in tcp_wmem_schedule()

This adds a packetdrill test that pushes sk_forward_alloc negative through
tcp_collapse() while sk_wmem_queued is above tcp_wmem[0]. It then checks
that a non-blocking write() under memcg pressure fails with EAGAIN. It also
adds REQUIRES_PAGESIZE and REQUIRES_MEMCG directives to ksft_runner.sh. The
REQUIRES_MEMCG directive enables the cgroup2 memory controller and installs
an exit cleanup handler.

> diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh
> index 0a97d5ae34699..ba62f3840eb57 100755
> --- a/tools/testing/selftests/net/packetdrill/ksft_runner.sh
> +++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh
> @@ -52,6 +52,34 @@ if [[ -n "${KSFT_MACHINE_SLOW}" ]]; then

[ ... ]

> +if grep -q -E '^--define=REQUIRES_MEMCG=' $script; then
> +	cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
> +	memcg_was_enabled=0
> +	if [[ -n "$cgroot" ]] &&
> +	   grep -qw memory "${cgroot}/cgroup.subtree_control" 2>/dev/null; then
> +		memcg_was_enabled=1
> +	fi
> +	if [[ -z "$cgroot" ]] ||
> +	   ! echo +memory > "${cgroot}/cgroup.subtree_control" 2>/dev/null; then
> +		ktap_skip_all "requires cgroup2 memory controller"
> +		exit "$KSFT_SKIP"
> +	fi

[Severity: Medium]
Is this check enough to decide that the test can run? It confirms that the
memory controller can be enabled at the root. It does not confirm that
socket memory is charged to the memcg.

If the kernel boots with cgroup.memory=nosocket, cgroup_memory_nosocket is
set. In that case mem_cgroup_css_online() never increments
memcg_sockets_enabled_key:

mm/memcontrol.c:mem_cgroup_css_online() {
    ...
    if (memcg_on_dfl && !cgroup_memory_nosocket)
        static_branch_inc(&memcg_sockets_enabled_key);
    ...
}

mem_cgroup_sk_alloc() then returns early:

    if (!mem_cgroup_sockets_enabled)
        return;

So the accepted socket gets no sk_memcg. The memory.high and memory.max
writes in step 4 of tcp_wmem_schedule_negative_fwd_alloc.pkt then have no
effect on it.

On such a kernel, wouldn't the final write(4, ..., 16384) succeed, so the
test reports FAIL instead of SKIP?

> +	cleanup_memcg() {
> +		rmdir "${cgroot}"/pkt_* 2>/dev/null || true
> +		if [[ "$memcg_was_enabled" -eq 0 ]]; then
> +			echo -memory > "${cgroot}/cgroup.subtree_control" \
> +				2>/dev/null || true
> +		fi
> +	}
> +	trap cleanup_memcg EXIT
> +fi

[Severity: Medium]
Can this rmdir remove cgroups that this runner did not create? The glob
matches every empty cgroup named pkt_* at the cgroup2 root. That root is
shared with the host, because the runner only does unshare -n.

Even a single run would delete an existing empty pkt_* cgroup on the host,
along with its configured limits.

When runners overlap, one runner's EXIT trap can also remove another test's
pkt_${PPID}. One example is kselftest's run_many() starting tests with
run_in_netns &. That cgroup is empty, and so removable, in two windows:

  - between the mkdir and the cgroup.procs write in the .pkt init command
  - after packetdrill moves itself back to its original cgroup following
    accept()

In either case, the step 4 writes to memory.high and memory.max would fail.

Could the cleanup be limited to the pkt_<pid> cgroups created by this
runner's children? The .pkt cleanup command already removes its own
"${cgroot}/pkt_${PPID}".

There is a similar question about memcg_was_enabled. It is sampled once,
before +memory is written. If another runner or another user starts using
the root memory controller while this test runs, the exit trap still writes
-memory to the root cgroup.subtree_control.

Is it intended to disable a shared controller based on that earlier
snapshot? Any EBUSY from that write is also silently dropped by the
|| true.

> +
>  ip_versions=$(grep -E '^--ip_version=' $script | cut -d '=' -f 2)
>  if [[ -z $ip_versions ]]; then
>  	ip_versions="ipv4 ipv6 ipv4-mapped-ipv6"
> diff --git a/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt b/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt
> new file mode 100644
> index 0000000000000..e4d8d13144f0a
> --- /dev/null
> +++ b/tools/testing/selftests/net/packetdrill/tcp_wmem_schedule_negative_fwd_alloc.pkt
> @@ -0,0 +1,116 @@

[ ... ]

> +`set -e
> +./defaults.sh
> +ip link set dev tun0 mtu 65535
> +sysctl -q net.ipv4.tcp_rmem="4096 131072 6291456"
> +sysctl -q net.ipv4.tcp_wmem="4096 16384 4194304"
> +taskset -cp 0 $PPID >/dev/null 2>&1 || true
> +cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
> +[ -n "$cgroot" ]
> +echo +memory > "${cgroot}/cgroup.subtree_control"
> +rmdir "${cgroot}/pkt_${PPID}" 2>/dev/null || true
> +mkdir "${cgroot}/pkt_${PPID}"
> +echo 0 > "${cgroot}/pkt_${PPID}/memory.swap.max" 2>/dev/null || true
> +echo $PPID > "${cgroot}/pkt_${PPID}/cgroup.procs"
> +`

[ ... ]

> +// 4. Drain per-CPU memcg stock via memory.high=0, cap memory.max=0 (O_NONBLOCK
> +//    avoids synchronous reclaim/OOM), and expect -EAGAIN on a 16 KB non-blocking
> +//    write. Because copy (16 KB) minus sk_forward_alloc (<= -16.4 KB) exceeds
> +//    PAGE_ALLOC_COSTLY_ORDER (32 KB), try_charge_memcg() fails without invoking
> +//    the memcg OOM killer.
> ++0.010 `cgroot=$(mount -t cgroup2 | head -1 | awk '{print $3}')
> +echo 0 > "${cgroot}/pkt_${PPID}/memory.high"
> +echo 0 > "${cgroot}/pkt_${PPID}/memory.high"
> +printf 0 | dd of="${cgroot}/pkt_${PPID}/memory.max" oflag=nonblock status=none`
> ++0.000 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> ++0.000 write(4, ..., 16384) = -1 EAGAIN (Resource temporarily unavailable)

[Severity: Medium]
How reliably does this empty the memcg's per-CPU stock before the final
write()?

If any stock for this memcg is left on the CPU running write(),
try_charge_memcg()->consume_stock() can cover the charge (about 9 pages)
without checking memory.max:

    if (nr_pages > MEMCG_CHARGE_BATCH ||
        !local_trylock(&memcg_stock.lock))
        return ret;

Each memory.high write calls drain_all_stock() once. drain_all_stock()
returns early if percpu_charge_mutex is contended. It also skips CPUs that
already have FLUSHING_CACHED_CHARGE set, and it drains only the current CPU
synchronously:

    if (!mutex_trylock(&percpu_charge_mutex))
        return;
    ...
            if (cpu == curcpu)
                drain_local_memcg_stock(&memcg_st->work);
            else
                schedule_drain_work(cpu, &memcg_st->work);

Remote CPUs are drained asynchronously, and nothing waits for them.

The memory.max write uses oflag=nonblock. That makes memory_max_write()
take the O_NONBLOCK path, which skips draining entirely:

    if (of->file->f_flags & O_NONBLOCK)
        goto out;

The synchronous local drain only reaches the right CPU if the earlier
taskset -cp 0 $PPID succeeded. A failure there is hidden by || true, for
example when CPU 0 is not in the runner's cpuset.

In those cases, could write() be served from leftover stock and succeed,
making the test flaky?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026032216.1145201-1-tavip%40google.com