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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3162 bytes --]

On Wed, Oct 08, 2025 at 12:42:49AM +0200, Stefano Brivio wrote:
> On Tue, 7 Oct 2025 10:34:01 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]