> On 20. Nov 2020, at 00:13, John Baldwin <j...@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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
