Re: [PATCH 4/5] tcp: Avoid SEQ_*() comparisons against 0

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 15 Jun 2026 18:18:32 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Usually we use the SEQ_{LT,LE,GT,GE}() comparison macros on raw sequence
> numbers, to see how they related handling possible wraparounds.  However,
> in two cases we take a difference of sequence numbers then compare that
> difference to 0.
> 
> As long as the difference is done using wrapping unsigned arithmetic,
> that's not incorrect.  It's essentially doing comparisons in a "shifted"
> space of sequence numbers, and I find working in both the raw and shifted
> spaces makes it harder to reason about.  Specifically I sometimes find I
> have to go through an additional layer of logic to convince myself that the
> wraparound handling is correct.
> 
> In addition to confusing me, it also tends to confuse cppcheck, at least in
> cppcheck 2.21.0 it often seems to trip false positives via cppcheck bug
> 14848.
> 
> It turns out that in each of the cases we use comparisons of sequence
> differences against 0, it's pretty much just as clear - maybe clearer to
> perform the comparisons against raw sequence numbers.  We still need the
> difference afterwards, but it's much more obvious that it means what we
> need it do, if we've already eliminated cases where the sequence numbers
> aren't in the expected order.
> 
> In the case in tcp_data_from_tap() this also lets us simplify the diagram
> explanation of the different cases a bit.  While there we correct the
> diagram in the second discard case: seq was shown as partway into the
> packet, which is not true by definition.  Instead seq_from_tap should be
> some distance after the end of the packet.

I have to admit I missed this paragraph and spent a disproportionate
amount of time trying to figure out why you would change that. Oops.
Good catch.

> Link: https://trac.cppcheck.net/ticket/14848
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index dcadc765..edcabe27 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1840,21 +1840,20 @@ static int tcp_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>  	uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
>  	uint32_t already_sent;
>  
> -	/* How much have we read/sent since last received ack ? */
> -	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
> -
> -	if (SEQ_LT(already_sent, 0)) {
> +	if (SEQ_LT(conn->seq_to_tap, conn->seq_ack_from_tap)) {
>  		/* RFC 761, section 2.1. */
>  		flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
>  			   conn->seq_ack_from_tap, conn->seq_to_tap);
>  		conn->seq_to_tap = conn->seq_ack_from_tap;
> -		already_sent = 0;
>  		if (tcp_set_peek_offset(conn, 0)) {
>  			tcp_rst(c, conn);
>  			return -1;
>  		}
>  	}
>  
> +	/* How much have we read/sent since last received ack ? */
> +	already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
> +
>  	if (!wnd_scaled || already_sent >= wnd_scaled) {
>  		conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
>  		conn_flag(c, conn, STALLED);
> @@ -1997,37 +1996,34 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>  		if (!len)
>  			continue;
>  
> -		seq_offset = seq_from_tap - seq;
>  		/* Use data from this buffer only in these two cases:
>  		 *
>  		 *      , seq_from_tap           , seq_from_tap
>  		 * |--------| <-- len            |--------| <-- len
> -		 * '----' <-- offset             ' <-- offset
>  		 * ^ seq                         ^ seq
> -		 *    (offset >= 0, seq + len > seq_from_tap)
> +		 *    (seq_from_tap >= seq, seq + len > seq_from_tap)
>  		 *
>  		 * discard in these two cases:
> -		 *          , seq_from_tap                , seq_from_tap
> +		 *          , seq_from_tap                   , seq_from_tap
>  		 * |--------| <-- len            |--------| <-- len
> -		 * '--------' <-- offset            '-----| <- offset
> -		 * ^ seq                            ^ seq
> -		 *    (offset >= 0, seq + len <= seq_from_tap)
> +		 * ^ seq                         ^ seq
> +		 *    (seq_from_tap >= seq, seq + len <= seq_from_tap)
>  		 *
>  		 * keep, look for another buffer, then go back, in this case:
>  		 *      , seq_from_tap
>  		 *          |--------| <-- len
> -		 *      '===' <-- offset
>  		 *          ^ seq
> -		 *    (offset < 0)
> +		 *    (seq_from_tap < seq)
>  		 */
> -		if (SEQ_GE(seq_offset, 0) && SEQ_LE(seq + len, seq_from_tap))
> +		if (SEQ_GE(seq_from_tap, seq) && SEQ_LE(seq + len, seq_from_tap))
>  			continue;
>  
> -		if (SEQ_LT(seq_offset, 0)) {
> +		if (SEQ_LT(seq_from_tap, seq)) {
>  			if (keep == -1)
>  				keep = i;
>  			continue;
>  		}
> +		seq_offset = seq_from_tap - seq;
>  
>  		iov_drop_header(&data, seq_offset);
>  		size = len - seq_offset;

-- 
Stefano