Ignore TCP SYN-ACK segments with non-matching SEG.ACK in SYN-SENT state

[tuexen]

Summary

When a SYN-ACK is received in SYN-SENT state, RFC 793 requires the validation of SEG.ACK as the first step.
If the ACK is not acceptable, the segment a RST segment is sent and the segment is dropped. Currently, the segment was partially processed.

This patch moves the check for the SEG.ACK validation up to the front as required.

Test Plan

Get the tests from tcp-testsuite passed.

[hiren]

For my understanding, what is the side-effect of this 'partial processing' before we eventually drop it? (I assume we do drop it but later in the path, right?)
Not against the patch but trying to understand what is driving this fix. :-)

[tuexen]

I observed the bug by using a SYN-ACK without any options. The retransmitted SYN does not contain the SACK option anymore. So it wasn't retransmitted.

[hiren]

In D10424#216131, @tuexen wrote:

I observed the bug by using a SYN-ACK without any options. The retransmitted SYN does not contain the SACK option anymore. So it wasn't retransmitted.

Can you please explain the scenario in a little more detail?

[tuexen]

The SUT sends a default SYN segment (including MSS option, SACK allowed, Timestamp and Window scaling). The tester responds with a SYN-ACK
without any options. In the tests I'm varying the SEG.ACK.

RFC 793 reads

If the state is SYN-SENT then

  first check the ACK bit

    If the ACK bit is set

      If SEG.ACK =< ISS, or SEG.ACK > SND.NXT, send a reset (unless
      the RST bit is set, if so drop the segment and return)

        <SEQ=SEG.ACK><CTL=RST>

      and discard the segment.  Return.

      If SND.UNA =< SEG.ACK =< SND.NXT then the ACK is acceptable.

When using SEG.ACK = SND.NXT+1 (see rcv-syn-ack-without-data-syn-sent-ack-outside-right-ipv4.pkt, TCP processes the incoming segment, disables the SACK extension and then drops it. Therefore, the retransmission of the SYN segment does not contain the SACK allowed option anymore. So the following is done before dropping the packet:

• tp->t_rcvtime is updated
• ECN handling performed (with updating CC)
• TCP option processing
• tp->rcv_wnd is updated

I think none of it should be done.

[hiren]

Thanks for the explanation. Change looks correct to me just that the code is a bit muddied with this single check moving elsewhere (for the right reasons) and a bit more code duplication in alt stack code. But I think those are out of the scope of this patch.