Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket

[Guillaume Nault <gnault@redhat.com>]

On Wed, Dec 04, 2019 at 07:08:49PM -0800, Eric Dumazet wrote:
> 
> 
> On 12/4/19 4:59 PM, Guillaume Nault wrote:
> > When no synflood occurs, the synflood timestamp isn't updated.
> > Therefore it can be so old that time_after32() can consider it to be
> > in the future.
> > 
> > That's a problem for tcp_synq_no_recent_overflow() as it may report
> > that a recent overflow occurred while, in fact, it's just that jiffies
> > has grown past 'last_overflow' + TCP_SYNCOOKIE_VALID + 2^31.
> > 
> > Spurious detection of recent overflows lead to extra syncookie
> > verification in cookie_v[46]_check(). At that point, the verification
> > should fail and the packet dropped. But we should have dropped the
> > packet earlier as we didn't even send a syncookie.
> > 
> > Let's refine tcp_synq_no_recent_overflow() to report a recent overflow
> > only if jiffies is within the
> > [last_overflow, last_overflow + TCP_SYNCOOKIE_VALID] interval. This
> > way, no spurious recent overflow is reported when jiffies wraps and
> > 'last_overflow' becomes in the future from the point of view of
> > time_after32().
> > 
> > However, if jiffies wraps and enters the
> > [last_overflow, last_overflow + TCP_SYNCOOKIE_VALID] interval (with
> > 'last_overflow' being a stale synflood timestamp), then
> > tcp_synq_no_recent_overflow() still erroneously reports an
> > overflow. In such cases, we have to rely on syncookie verification
> > to drop the packet. We unfortunately have no way to differentiate
> > between a fresh and a stale syncookie timestamp.
> > 
> > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > ---
> >  include/net/tcp.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index f0eae83ee555..005d4c691543 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -520,12 +520,14 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk)
> >  		if (likely(reuse)) {
> >  			last_overflow = READ_ONCE(reuse->synq_overflow_ts);
> >  			return time_after32(now, last_overflow +
> > -					    TCP_SYNCOOKIE_VALID);
> > +					    TCP_SYNCOOKIE_VALID) ||
> > +				time_before32(now, last_overflow);
> >  		}
> >  	}
> >  
> >  	last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
> > -	return time_after32(now, last_overflow + TCP_SYNCOOKIE_VALID);
> > +	return time_after32(now, last_overflow + TCP_SYNCOOKIE_VALID) ||
> > +		time_before32(now, last_overflow);
> >  }
> 
> 
> There is a race I believe here.
> 
> CPU1                                 CPU2
>  
> now = jiffies.
>     ...
>                                      jiffies++
>                                      ...
>                                      SYN received, last_overflow is updated to the new jiffies.
> 
> 
> CPU1 
>  timer_before32(now, last_overflow) is true, because last_overflow was set to now+1
> 
> 
> I suggest some cushion here.
> 
Yes, we should wrap access to ->rx_opt.ts_recent_stamp into READ_ONCE(),
to ensure that last_overflow won't be reloaded between the
time_after32() and the time_before32() calls. Is that what you had in
mind?

-	last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
+	last_overflow = READ_ONCE(tcp_sk(sk)->rx_opt.ts_recent_stamp);

Patch 1 would need the same fix BTW.

> Also we TCP uses between() macro, we might add a time_between32(a, b, c) macro
> to ease code review.
> 
I didn't realise that. I'll define it in v3.

> ->
>   return !time_between32(last_overflow - HZ, now, last_overflow + TCP_SYNCOOKIE_VALID);
> 
'last_overflow - HZ'? I don't get why we'd take HZ into account here.