From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=ivN+KpqE; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 14FC15A0272 for ; Mon, 15 Jun 2026 10:18:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1781511516; bh=kC07zmcjwa5G5O2+Mmy/O5T71/F/yU++FG7B2Ar1zws=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=ivN+KpqE94upJ2hOFJ1RI3HAPaKIHl9ONol7OGEv15oQx9bvJ8sMZH1hc5+MpRiae afU4oJzxEDgZWbIuYnQ8L1u5Cm9S5nSvfbhLxvjvdY0RVfAktEu3RTBf5/s0jSyeQq l0rF+Jh4yxbSCzUZq5MbxE1dmaG/jZZw2oqSzbmRv/9NjxX0/miMCuD9AIMDhJvNP5 hpY4CfDHy9gQtF4wnGwucgzMA0dETUis/V/fPIUZmWFSqciinvaOHlgLjoplyH/c9j 7d2QwuFkqAzQ8zML1W1TZ91TsmvSb7PlNy8jMk7/KvfnmjFJLVF0DAToOp2xzDrJzz m5sY0elDVcXTQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gf30r1rfZz4wTS; Mon, 15 Jun 2026 18:18:36 +1000 (AEST) From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: [PATCH 4/5] tcp: Avoid SEQ_*() comparisons against 0 Date: Mon, 15 Jun 2026 18:18:32 +1000 Message-ID: <20260615081833.1061725-5-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.54.0 In-Reply-To: <20260615081833.1061725-1-david@gibson.dropbear.id.au> References: <20260615081833.1061725-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: BMXXTQWINJUXKRFLAQDSANYX4XIBKWPR X-Message-ID-Hash: BMXXTQWINJUXKRFLAQDSANYX4XIBKWPR X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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. Link: https://trac.cppcheck.net/ticket/14848 Signed-off-by: David Gibson --- 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; -- 2.54.0