Re: [PATCH 3/4] tcp: Don't consider FIN flags with mismatching sequence

On Wed, Oct 08, 2025 at 12:42:49AM +0200, Stefano Brivio wrote:

On Tue, 7 Oct 2025 10:34:01 +1100 David Gibson wrote:

...

On Tue, Oct 07, 2025 at 12:32:54AM +0200, Stefano Brivio wrote:

...

On Fri, 3 Oct 2025 13:43:32 +1000 David Gibson wrote: [snip]

...

@@ -1820,6 +1817,8 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn, break; seq_from_tap += size; iov_i += count; + if (th->fin) + fin = 1;

if (keep == i) keep = -1;

We'd need to double check that the "accept data segment" path is safe with len == 0, of course.

For sure it's not before d2c33f45f7be ("tcp: Convert tcp_data_from_tap() to use iov_tail"), because we might add zero-length segments to the tcp_iov array, and that would make backporting an otherwise simple and critical fix to slightly older versions rather complicated.

Kinda. It's not that complicated to deal with that case, by wrapping the actual data processing in an `if (len) { ... }`

That's needed for sure, but do we risk looping forever on a particular batch of FIN segments without data with a given series of sequence numbers?

Right now the loop terminates because the sequence moves forward. I'm not sure what happens without data while moving 'keep' around. Maybe it takes a few minutes to figure out (I haven't tried) but I wouldn't call that trivial.

That should be fine, because we need to advance the sequence by one for the FIN anyway, so we will move forwards. I might have forgotten that in my quick example, which is a bug, but an easy one to fix.

...

...

After that commit, I'm not sure about the behaviour of iov_tail_clone(). I think it will return 0, but it should be tested (that assumption, itself, but also that the fix still works).

Right, I think it's safe in that case - that's what I was looking at.

Note that I didn't test this.

Understood.

...

...

...

But I think that will treat dataless and with-data FINs the same way, and let them use the keep mechanism.

Given that the only advantage of doing this would be to handle a rare (I guess) corner case, that is, an out-of-sequence FIN segment with data, which is not critical anyway because the FIN will be retransmitted, I would rather keep this (critical) fix as it is.

I would suggest filing a separate ticket or anyway sending a separate patch if you want to fix that other case.

Fair enough. I'll wait for you to get the basic fix merge, then I might batch this cleanup/fixup with the reorg to clarify bytes_acked handling.

Okay, thanks, merged now.

-- Stefano

-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson