From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=gvf6b9/j; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 007035A0262 for ; Wed, 17 Jun 2026 01:08:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781651322; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vDJNjcbCO1fK+1rAUOzl1gO6jqpX/UwtXQGqWg71CeU=; b=gvf6b9/jhXCAhbrIf9g6QAhv6Mf9PiSSgmTv376Sl41fw8ME2f8Kvop/alRlpcTZVYq6L2 kunHSv1E3wMmCXS4nJNaa6aOqzSSWSU6gQPUqcA0sZslNykMqvbdmyyOs3YMJlzGdBaU6e gz0nzTzDKIst2n8kdnT7hCBVH88eJ0U= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-491-Pgm-bbIzM4uUi98_RVHSqw-1; Tue, 16 Jun 2026 19:08:41 -0400 X-MC-Unique: Pgm-bbIzM4uUi98_RVHSqw-1 X-Mimecast-MFC-AGG-ID: Pgm-bbIzM4uUi98_RVHSqw_1781651320 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-490aadb1386so1571475e9.0 for ; Tue, 16 Jun 2026 16:08:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781651320; x=1782256120; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vDJNjcbCO1fK+1rAUOzl1gO6jqpX/UwtXQGqWg71CeU=; b=ZWGvP7L4WVK5g/abCHdzwP/1+ftxAF8QmBS5vL5v95EnZB+wpL3JQvx8DE/UoLx1aL YtT08OSZ7atxCikeq4+wdH02IgkbjGCofkhJy2nOVG14OQ8Gnuoflpm1bHAlVpRlHSDQ Gn4DbD0EI+oJ8qCML8IYhqVz4nOBe6ZQixz2+6xlzPtfRD/lRf8T4mvV3IaKnO94FPSY piEG0wVc7HliEALpkDlm75/ov0a63rBjMn+ga/KyonNYzy0c2/eeDMYz6eaVzTDP742R g8ZHtO2aOKyzT7bYGFbE+jdq1JZw2kGxMHoeq/JbZUqUIR1JRii3rK2pRgIL2IrmLa1c AWeg== X-Gm-Message-State: AOJu0Yz2u//AejnDWrYug5UHm3T+bHICuKj8+kY4wUCj/b0CR7Jc+KRl R31zAqRGbk4hUGXLzSO8J9bPgSwW04hPaFLrtIfhTbb/go493/CzBmwii16DBGFjs2UqMBlL8xf i01Y4CWGXpn/mJ1/WY5dP/VG9Ir7KcXA1nH0QlsYBNw7yKu8/z4qXnnFW6ovJ8w== X-Gm-Gg: Acq92OEEKSLPjSrilwXFAkTAsOWgcdlCyK1IboFwEU5Jq8siQ20Ju0+rShXFfdDngKd 8geUK1u+RkayMUV8qS5lRcYs9YUuOrv8XtL3uUDItO8eWGBWYMEXjLGVrUkDFTFWxy6bqM8fd5l sdGItXeGa/M6JKRFrAcNxMyV1+c3fi7SSKcPSJ4G0BAE1d/roeTnUGmKs21/rLMCRmc/pq4j3Xq lq9rlPVNODGKjzp9GYU8S3YvflpgkoNTiorxpgbKUVozKSfmNF3EV8LHG+t8dtbn1aOQX32Eyxy bZLvLhGT57di+0CTHtustGqMqOLUD4nkKcx98ZvNrod1V4ZTd1IhzrVPIkjEg2cVcfOzjJwFUJY 28zlmJIplgKXi0LZuAN3/iBKnjDKjExYO X-Received: by 2002:a05:600c:a088:b0:490:b26c:64ad with SMTP id 5b1f17b1804b1-492340c5e3amr9280015e9.5.1781651319933; Tue, 16 Jun 2026 16:08:39 -0700 (PDT) X-Received: by 2002:a05:600c:a088:b0:490:b26c:64ad with SMTP id 5b1f17b1804b1-492340c5e3amr9279685e9.5.1781651319363; Tue, 16 Jun 2026 16:08:39 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49233bed5d3sm11073475e9.2.2026.06.16.16.08.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2026 16:08:38 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 4/5] tcp: Avoid SEQ_*() comparisons against 0 Message-ID: <20260617010837.41032877@elisabeth> In-Reply-To: <20260615081833.1061725-5-david@gibson.dropbear.id.au> References: <20260615081833.1061725-1-david@gibson.dropbear.id.au> <20260615081833.1061725-5-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 17 Jun 2026 01:08:38 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 2uG_zOXkDd9LaF44CSB5hMTCCp1xKDaEcAJScUb0PM4_1781651320 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 3ZVPQ3FG7KZSKE6MBC6HFGMWJULYW4JW X-Message-ID-Hash: 3ZVPQ3FG7KZSKE6MBC6HFGMWJULYW4JW X-MailFrom: sbrivio@redhat.com 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: passt-dev@passt.top 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: On Mon, 15 Jun 2026 18:18:32 +1000 David Gibson 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 > --- > 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