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

[Kuniyuki Iwashima <kuniyu@google.com>]

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);