svn commit: r367530 - in head/sys/netinet: . tcp_stacks

[John Baldwin]

On 11/19/20 2:55 PM, John Baldwin wrote:
> On 11/9/20 1:49 PM, Michael Tuexen wrote:
>> Author: tuexen
>> Date: Mon Nov  9 21:49:40 2020
>> New Revision: 367530
>> URL: https://svnweb.freebsd.org/changeset/base/367530
>>
>> Log:
>>   RFC 7323 specifies that:
>>   * TCP segments without timestamps should be dropped when support for
>>     the timestamp option has been negotiated.
>>   * TCP segments with timestamps should be processed normally if support
>>     for the timestamp option has not been negotiated.
>>   This patch enforces the above.
>>   
>>   PR:			250499
>>   Reviewed by:		gnn, rrs
>>   MFC after:		1 week
>>   Sponsored by:		Netflix, Inc
>>   Differential Revision:	https://reviews.freebsd.org/D27148
>>
>> Modified:
>>   head/sys/netinet/tcp_input.c
>>   head/sys/netinet/tcp_stacks/bbr.c
>>   head/sys/netinet/tcp_stacks/rack.c
>>   head/sys/netinet/tcp_syncache.c
>>   head/sys/netinet/tcp_timewait.c
>>
>> Modified: head/sys/netinet/tcp_timewait.c
>> ==============================================================================
>> --- head/sys/netinet/tcp_timewait.c	Mon Nov  9 21:19:17 2020	(r367529)
>> +++ head/sys/netinet/tcp_timewait.c	Mon Nov  9 21:49:40 2020	(r367530)
>> @@ -376,7 +376,7 @@ tcp_twstart(struct tcpcb *tp)
>>   * looking for a pcb in the listen state.  Returns 0 otherwise.
>>   */
>>  int
>> -tcp_twcheck(struct inpcb *inp, struct tcpopt *to __unused, struct tcphdr *th,
>> +tcp_twcheck(struct inpcb *inp, struct tcpopt *to, struct tcphdr *th,
>>      struct mbuf *m, int tlen)
>>  {
>>  	struct tcptw *tw;
>> @@ -410,6 +410,16 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to __unu
>>  	 */
>>  	if (thflags & TH_RST)
>>  		goto drop;
>> +
>> +	/*
>> +	 * If timestamps were negotiated during SYN/ACK and a
>> +	 * segment without a timestamp is received, silently drop
>> +	 * the segment.
>> +	 * See section 3.2 of RFC 7323.
>> +	 */
>> +	if (((to->to_flags & TOF_TS) == 0) && (tw->t_recent != 0)) {
>> +		goto drop;
>> +	}
> 
> This causes an insta-panic with TOE because toe_4tuple_check() passes in a NULL
> pointer for 'to'.  I'm working on a fix for that, but perhaps wait to MFC until
> the fix is ready so they can be merged together?
> 
> That said, TOE only calls this in the case that it has gotten a new SYN, so I
> wonder if it makes sense to apply this check on a new SYN.  For a new SYN,
> shouldn't we not care if the new connection is using a different timestamp
> option from the old connection?  The language in RFC 7323 3.2 is all about
> segments on an existing connection, not segments from a new connection I think?
> 
> That is, I think we should perhaps move this check after the TH_SYN check so
> that a mismatch doesn't prevent recycling?

Actually, we move the check below requiring TH_ACK, I think this would fix the TOE
case and also DTRT for plain SYNs for non-TOE:

diff --git a/sys/netinet/tcp_timewait.c b/sys/netinet/tcp_timewait.c
index c52eab956303..85f1ccbe40f9 100644
--- a/sys/netinet/tcp_timewait.c
+++ b/sys/netinet/tcp_timewait.c
@@ -411,16 +411,6 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to, struct tcphdr *th,
 	if (thflags & TH_RST)
 		goto drop;
 
-	/*
-	 * If timestamps were negotiated during SYN/ACK and a
-	 * segment without a timestamp is received, silently drop
-	 * the segment.
-	 * See section 3.2 of RFC 7323.
-	 */
-	if (((to->to_flags & TOF_TS) == 0) && (tw->t_recent != 0)) {
-		goto drop;
-	}
-
 #if 0
 /* PAWS not needed at the moment */
 	/*
@@ -455,6 +445,16 @@ tcp_twcheck(struct inpcb *inp, struct tcpopt *to, struct tcphdr *th,
 	if ((thflags & TH_ACK) == 0)
 		goto drop;
 
+	/*
+	 * If timestamps were negotiated during SYN/ACK and a
+	 * segment without a timestamp is received, silently drop
+	 * the segment.
+	 * See section 3.2 of RFC 7323.
+	 */
+	if (((to->to_flags & TOF_TS) == 0) && (tw->t_recent != 0)) {
+		goto drop;
+	}
+
 	/*
 	 * Reset the 2MSL timer if this is a duplicate FIN.
 	 */

The commented out PAWS bits would also seem to not be relevant for SYN-only
packets?  However, I'm less sure of if that bit should be moved later as
well. (Or perhaps it should just be removed.  It has been #if 0'd since the
timewait structure was first added back in 2003 by jlemon@)

-- 
John Baldwin