Re: [PATCH v1 net-next 6/8] tcp: Allocate all new skb before collapsing.

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] In tcp_collapse() (net/ipv4/tcp_input.c), the new
  TCP_COLLAPSE_MAX_TOTAL_SIZE cap sits inside the second operand of…
- [Low] Concern: preallocating every skb makes tcp_collapse() all-or-
  nothing per section.

--

tcp: Allocate all new skb before collapsing.

This commit changes tcp_collapse() to preallocate every replacement skb
for a section into a temporary pool before copying any data. An
alloc_skb() failure therefore no longer leaves partially-copied skbs and
duplicates in the queue. To bound the batch, the section size is capped at
TCP_COLLAPSE_MAX_TOTAL_SIZE (16 skbs). The exceptions are a single bloated
skb and overlapping OOO skbs.

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index fe71305f6e673..9f32577b0b53c 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c

[ ... ]

> @@ -5908,8 +5911,9 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
>  
>  		/* seq and end_seq must increase monotonically for rbtree. */
>  		if ((list || !before(TCP_SKB_CB(n)->seq, sub_end)) &&
> -		    !tcp_collapse_skb_bloated(n, sub_end,
> -					      TCP_COLLAPSE_SCALING_RATIO))
> +		    (TCP_SKB_CB(n)->end_seq - start >= TCP_COLLAPSE_MAX_TOTAL_SIZE ||
> +		     !tcp_collapse_skb_bloated(n, sub_end,
> +					       TCP_COLLAPSE_SCALING_RATIO)))
>  			break;
>  
>  		sub_end = TCP_SKB_CB(n)->end_seq;

[Severity: Medium]

Can the TCP_COLLAPSE_MAX_TOTAL_SIZE cap ever stop the section on the OOO
overlap path?

For the OOO rbtree (list == NULL), suppose n partially overlaps the
section, so n->seq < sub_end and n->end_seq > sub_end. Then the first
operand of the && is false. Neither the cap nor tcp_collapse_skb_bloated()
is checked, and sub_end keeps growing.

The peer controls seq and end_seq. tcp_data_queue_ofo() keeps partially
overlapping segments in the tree:

net/ipv4/tcp_input.c:tcp_data_queue_ofo() {
    ...
			if (after(seq, TCP_SKB_CB(skb1)->seq)) {
				/* Partial overlap. */
				tcp_dsack_set(sk, seq, TCP_SKB_CB(skb1)->end_seq);
			} else {
    ...
}

So a chain of segments, each starting just before the previous one ends,
can turn into a single section. Its size is limited only by the OOO memory
of the socket.

Only the first skb has to be bloated. The overlap also inflates
old_truesize, so the old_truesize > new_truesize check is easy to pass.

> @@ -5932,14 +5938,19 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
>  		goto restart;
>  	}
>  
> -	while (before(start, sub_end)) {
> -		int copy = min_t(int, TCP_COLLAPSE_MAX_SIZE, sub_end - start);
> -		struct sk_buff *nskb;
> -
> -		nskb = alloc_skb(copy, GFP_ATOMIC);
> +	while (nskbs--) {
> +		nskb = alloc_skb(nskbs ? TCP_COLLAPSE_MAX_SIZE : remainder,
> +				 GFP_ATOMIC);
>  		if (!nskb)
>  			goto end;
>  
> +		__skb_queue_tail(&pool, nskb);
> +	}

This loop then allocates every replacement skb with GFP_ATOMIC. All of
them sit in pool before any source skb is freed or any new skb is charged
to the socket.

Before this patch, alloc_skb() alternated with
tcp_collapse_one()->__kfree_skb(), so only about one extra skb was in
flight at a time. Now the number in flight is about
ceil((sub_end - start) / TCP_COLLAPSE_MAX_SIZE). With one-byte overlaps and
typical MSS truesize, that is roughly 0.7x the OOO memory of the socket,
on top of the existing queue.

A larger all-or-nothing batch is also more likely to fail under the memory
pressure that triggers collapse. In that case goto end frees the pool and
tcp_prune_queue() falls back to tcp_prune_ofo_queue().

The commit message does mention this exception:

  except when a single bloated skb or overlapping OOO skbs
  must be collapsed together to free the original skb or keep the
  rbtree monotonic.

However, the size of the overlapping OOO case is controlled by the peer
and bounded only by the OOO queue. Should the overlap path be bounded as
well? If not, could the commit message say that this exception is not
covered by the bound on the temporary allocation?

[Severity: Low]

Each section is now all-or-nothing. A single alloc_skb() failure in this
loop jumps to end, and nothing is reclaimed for this section or for any
later one. Even in the capped case, up to 16 skbs (about 70 KiB on 4 KiB
pages) are held before any source skb is freed.

The unused skbs are released by __skb_queue_purge_reason() at end, so
nothing leaks.

The commit message describes this tradeoff ("It is only when all
candidates are properly collapsed that we can save memory" and "a small
tradeoff to bound the temporary allocation"). The goto end on allocation
failure was also already there before this patch.

Is it intended that one failed batch also skips the remaining sections,
rather than only the current one?

> +
> +	while (before(start, sub_end)) {
> +		int copy = min_t(int, TCP_COLLAPSE_MAX_SIZE, sub_end - start);
> +
> +		nskb = __skb_dequeue(&pool);

[ ... ]

> @@ -5985,6 +5996,8 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
>  end:
>  	skb_queue_walk_safe(&tmp, skb, n)
>  		tcp_rbtree_insert(root, skb);
> +
> +	__skb_queue_purge_reason(&pool, SKB_CONSUMED);
>  }

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