Re: [PATCH v1 net-next 1/8] tcp: Clean up the first loop in tcp_collapse().

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch is titled a clean-up, and its message presents the
  change as equivalent to the old code ("If we goto skip_this in both…

--

tcp: Clean up the first loop in tcp_collapse().

This commit restructures the first loop of tcp_collapse(). The SYN/FIN
and skb_frags_readable() checks are folded into one goto skip_this test,
and the end_of_skbs flag is replaced with the inverted loop condition.
It also moves the readability check after the fully overlapping check.

> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index a2074a1af4222..32e63d089db89 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5839,23 +5839,17 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,

[ ... ]

>  restart:
> -	for (end_of_skbs = true; skb != NULL && skb != tail; skb = n) {
> +	for (; skb && skb != tail; skb = n) {
>  		n = tcp_skb_next(skb, list);
>  
> -		if (!skb_frags_readable(skb))
> -			goto skip_this;
> -
>  		/* No new bits? It is possible on ofo queue. */
>  		if (!before(start, TCP_SKB_CB(skb)->end_seq)) {
>  			skb = tcp_collapse_one(sk, skb, list, root);
> -			if (!skb)
> -				break;
>  			goto restart;
>  		}
>  
> @@ -5864,27 +5858,25 @@ tcp_collapse(struct sock *sk, struct sk_buff_head *list, struct rb_root *root,
>  		 * - bloated or contains data before "start" or
>  		 *   overlaps to the next one and mptcp allow collapsing.
>  		 */
> -		if (!(TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)) &&
> -		    (tcp_win_from_space(sk, skb->truesize) > skb->len ||
> -		     before(TCP_SKB_CB(skb)->seq, start))) {
> -			end_of_skbs = false;
> +		if (!skb_frags_readable(skb) ||
> +		    TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN))
> +			goto skip_this;

[Severity: Low]
The commit message describes this as an equivalent clean-up:

  If we goto skip_this in both cases, we can remove the second
  tests and simply replace end_of_skbs with the inverted loop
  condition.

Is that true for SYN/FIN skbs? Before this patch, take a SYN or FIN skb
whose readable successor n overlaps it and is allowed by
tcp_skb_can_collapse_rx(). That skb reached the second break with
end_of_skbs = false:

	if (n && n != tail && skb_frags_readable(n) &&
	    tcp_skb_can_collapse_rx(skb, n) &&
	    TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(n)->seq) {
		end_of_skbs = false;
		break;
	}

The post-loop test then returned early:

	if (end_of_skbs ||
	    (TCP_SKB_CB(skb)->tcp_flags & (TCPHDR_SYN | TCPHDR_FIN)) ||
	    !skb_frags_readable(skb))
		return;

So nothing in the head..tail range was collapsed in that call. The
post-loop SYN/FIN test was not redundant.

With this patch every SYN/FIN skb goes to skip_this. start advances and
scanning continues, so later skbs in the same range can now be collapsed.
A peer can reach this path, for example with a FIN segment followed by an
overlapping segment in the ofo queue.

The readability reordering also changes behaviour. Unreadable (devmem)
skbs that lie entirely below start used to be skipped. Now they are
unlinked and freed by tcp_collapse_one(), which also increments
LINUX_MIB_TCPRCVCOLLAPSED. The message only says:

  Also, skb_frags_readable() should be tested after the fully
  overlapping check.

It gives no reason for the change.

The new behaviour looks safe as far as I can tell. The copy loop still
stops at SYN/FIN skbs, and offset stays non-negative.

Could the changelog state both the SYN/FIN change and the devmem change
explicitly, rather than presenting the patch as a pure clean-up? The first
loop at the end of the series still behaves this way, so this patch looks
like the right place to describe it.

> +
> +		if (tcp_win_from_space(sk, skb->truesize) > skb->len ||
> +		    before(TCP_SKB_CB(skb)->seq, start))
>  			break;
> -		}

[ ... ]

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