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

[Michael Tuexen]

> On 20. Nov 2020, at 00:13, John Baldwin <jhb at freebsd.org> wrote:
> 
> 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:
Let me have a look tomorrow morning...

Best regards
Michael
> 
> 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