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

[John Baldwin]

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?

-- 
John Baldwin