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: 3016 bytes --]

On Wed, Oct 08, 2025 at 12:01:27PM +0200, Stefano Brivio wrote:
> On Wed, 8 Oct 2025 11:41:23 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> 
> But we don't, in that loop, and there's a specific reason for it. FIN
> segments are a special case in that, if you receive more than one, you
> don't advance the sequence by more than one, even if the segments
> themselves are in the expected sequence.

Ok.  *thinks*.  So, if we both set fin = 1 and advance the sequence
inside an if (th->fin && !fin), that should do the trick, yes?

> If you want to move the sequence increase into that loop (which might
> make sense with some extra care), perhaps it's worth doing that
> together with the whole https://bugs.passt.top/show_bug.cgi?id=125 at
> this point.

Yeah, maybe.

-- 
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 --]