[PATCH net v5] tcp: do not let tcp_collapse() drive sk_forward_alloc negative

[PATCH net v5] tcp: do not let tcp_collapse() drive sk_forward_alloc negative

[Octavian Purdila]

From: Eric Dumazet <edumazet@kernel.org>

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")
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
Signed-off-by: Octavian Purdila <tavip@google.com>
---
Changes in v5:
- Replace clamping in tcp_wmem_schedule() with Eric's patch calling
  sk_forced_mem_schedule(sk, nskb->truesize) in tcp_collapse() so
  sk_forward_alloc cannot become negative in the first place and the TX
  fast path is untouched.
- Defer the packetdrill selftest to net-next alongside the upcoming
  broader tcp_collapse() improvements.
- Link to v4: https://lore.kernel.org/netdev/20261026032216.1145201-1-tavip@google.com/

 net/ipv4/tcp_input.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 99baf14afdfd8..24d5f016a4eb6 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5846,6 +5846,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);
 
-- 
2.56.0.313.g2479322e68-goog

Re: [PATCH net v5] tcp: do not let tcp_collapse() drive sk_forward_alloc negative

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v5] tcp: do not let tcp_collapse() drive sk_forward_alloc negative

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · 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).

- [High] The fix is incomplete.
- [High] The fix is incomplete.

--

tcp: do not let tcp_collapse() drive sk_forward_alloc negative

This patch makes tcp_collapse() call sk_forced_mem_schedule() before it
charges each newly allocated nskb with skb_set_owner_r(). Without that,
sk->sk_forward_alloc can go negative, because the new skbs are charged
before the skbs they replace are freed. A negative sk_forward_alloc can
make tcp_wmem_schedule() return a negative length, which trips the
WARN_ON_ONCE() in check_copy_size().

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 1ddce9c4b7e8c..e879bdfddf1ab 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);

[Severity: High]
Does tcp_data_queue_ofo() in net/ipv4/tcp_input.c have the same problem?

The v5 notes say this change makes it so "sk_forward_alloc cannot become
negative in the first place". The out-of-order queue path seems to do
things in the same order as tcp_collapse(): it frees older charged skbs
and then charges the new one.

First, it reserves the truesize of the incoming skb:

net/ipv4/tcp_input.c:tcp_data_queue_ofo() {
    ...
	if (unlikely(tcp_try_rmem_schedule(sk, skb, skb->truesize))) {
    ...
}

Before the incoming skb is charged, any OFO skbs it covers are dropped.
This happens both in the "skb covers skb1" replace branch and in the
merge_right loop:

		tcp_drop_reason(sk, skb1, SKB_DROP_REASON_TCP_OFOMERGE);

Each skb1 still has sock_rfree() as its destructor, so this goes through
sock_rfree()->sk_mem_uncharge()->sk_mem_reclaim(). With sk_reserved_mem
== 0, sk_mem_reclaim() gives back every whole page, including the pages
that were just reserved for the incoming skb.

The incoming skb is then charged without a new reservation:

end:
	if (skb) {
		...
		skb_condense(skb);
		skb_set_owner_r(skb, sk);
	}

This leaves sk_forward_alloc at ((F + T1) mod PAGE_SIZE) - T, where F is
the old sk_forward_alloc, T1 is the truesize of the freed skb and T is
the truesize of the incoming skb. For example, F = 0, T1 = 2000 and
T = 3000 give -1000. The result is always negative when T > PAGE_SIZE,
for example with a GRO skb that skb_condense() leaves alone.

The peer controls the OFO layout. It only needs to send a segment that
starts at the same sequence as a queued OFO segment, or one that covers
later OFO segments.

Suppose tcp_sendmsg_locked() runs after this while sk_wmem_schedule() is
failing (memcg charge failure or tcp_mem limits) and sk_wmem_queued >=
tcp_wmem[0]. Can tcp_wmem_schedule() still return a negative value here?

net/ipv4/tcp.c:tcp_wmem_schedule() {
    ...
	return min(copy, sk->sk_forward_alloc);
}

That would reach WARN_ON_ONCE(bytes > INT_MAX) in check_copy_size()
through skb_copy_to_page_nocache()->copy_from_iter_full_nocache().

Would it make sense to also call sk_forced_mem_schedule(sk, skb->truesize)
before skb_set_owner_r() in tcp_data_queue_ofo()? Two alternatives are
charging the skb before the covered skbs are dropped, or keeping the
sink-side clamp in tcp_wmem_schedule().

[Severity: High]
Can sk_forward_alloc also go negative through the non-pure MSG_ZEROCOPY
path in tcp_sendmsg_locked()?

v5 replaced the v4 clamp in tcp_wmem_schedule() with this source-side
fix. That leaves this return in tcp_wmem_schedule() relying on
sk_forward_alloc >= 0:

	return min(copy, sk->sk_forward_alloc);

When the tail skb is not pure zerocopy, tcp_sendmsg_locked() only
schedules copy bytes:

			if (!skb_zcopy_pure(skb)) {
				copy = tcp_wmem_schedule(sk, copy);
				if (!copy)
					goto wait_for_space;
			}

Next, the call chain
skb_zerocopy_iter_stream()->__zerocopy_sg_from_iter()->zerocopy_fill_skb_from_iter()
grows truesize by whole pages:

net/core/datagram.c:zerocopy_fill_skb_from_iter() {
    ...
		skb->truesize += PAGE_ALIGN(copied + start);
    ...
}

__zerocopy_sg_from_iter() then charges that truesize without scheduling
it:

		if (!skb_zcopy_pure(skb))
			sk_mem_charge(sk, truesize);

Take sk_forward_alloc = 3000, copy = 2000 and a user buffer at page
offset 3000. The data spans two pages, so 8192 bytes are charged and
sk_forward_alloc ends up at -5192. Several small iovecs on different
pages make the overcharge larger.

Suppose a later sk_wmem_schedule() fails while sk_wmem_queued >=
tcp_wmem[0]. Then tcp_wmem_schedule() returns a negative value, and the
if (!copy) checks in tcp_sendmsg_locked() do not catch it.

On the copy path, that reaches the same check_copy_size() WARN that the
commit message describes. On the non-pure zerocopy path, the negative int
becomes a huge size_t length in __zerocopy_sg_from_iter() and bypasses
size_goal. On the MSG_SPLICE_PAGES path, skb_splice_from_iter() is given
a negative maxsize.

Should the v4 sink-side guard in tcp_wmem_schedule() be kept along with
this tcp_collapse() change?

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