Ignore ACKs with tsecr 0 during 3whs in syncache code.

[hiren]

Summary

For RTT calculations mid-session, we explicitly ignore ACKs with tsecr of 0 as
many borken middle-boxes tend to do that. But during 3whs, in syncache_expand(),
we don't do that which causes us to send a RST to such a client. Fix this
behavior by only using tsecr to compare against timestamp that we sent when it
is not 0.

Looks something like this:
15:56:18.908705 IP client > server: Flags [S], seq 166485759, win 14600, options [mss 1460,sackOK,TS val 4804245 ecr 0,nop,wscale 7], length 0
15:56:18.908715 IP server > client: Flags [S.], seq 1317800117, ack 166485760, win 65535, options [mss 1460,nop,wscale 4,sackOK,TS val 2375093244 ecr 4804245], length 0
15:56:19.001364 IP client > server: Flags [.], ack 1, win 115, options [nop,nop,TS val 4804254 ecr 0], length 0
15:56:19.001372 IP server > client: Flags [R], seq 1317800118, win 0, length 0

Here, we send RST because tsecr of 3rd ACK is not reflecting tsval we send in SYN+ACK we sent.

Patch here relaxes this requirement when tsecr is (known broken) 0.

[jtl]

Hiren,

Thanks for finding this bug and proposing a solution! Your solution looks fine in the sense that it mimics the rest of the behavior of this function when validation fails.

However, I think this function (and tcp_input(), which calls it) actually do the wrong thing. I believe the correct behavior should be:

• If we have a matching and valid syncache entry, setup the connection.
• If we do not find a matching connection, send a RST.
• If we find a matching syncache entry, but there is a problem with validating it (e.g. RST, etc.), ignore it.

The consequence of the current behavior is that an errant TCP packet can tear down a connection attempt.

Of course, I'm not sure my proposed behavior is much better. It has at least two problems:

• How do you apply these rules to syncookies?
• An errant SYN will prevent another connection from the same host/port until the syncache entry times out.

So, my proposed behavior is probably not fully correct, either.

In any case, I just wanted to take this opportunity to note that I think we should have a conversation about the correct behavior here.

Jonathan

[jason_eggnet.com]

Jonathan,

I think the problems with the current behavior are a bit larger than a RST. As far as we can tell, we send a RST, then delete the syncache entry, which allows the request packet (for http anyway) to create a session... because syncookies were in use.

Sometimes, the RST gets dropped by the network, and in the above scenario, both sides of the connection proceed as if nothing happened. It is a bit bizarre.

So, in short, I generally agree that not only should we not be sending a RST in the case of a syncache hit but some other problem, we should also not delete the syncache entry.

I don't think syncookies should complicate things too much. If there is a matching syncache entry then the fact that syncookies are in use should be irrelevant. If there is no syncache entry then you're stuck validating whatever syncookies will let you validate.

Having said that, it appears we do need an exception for 0 ecr in the 3whs, because enough clients (or more likely middleboxes) are sending them.

This is still early in the investigation, so take the above with a grain of salt :)

Thanks,
-Jason

[hiren]

In D8552#177689, @jtl wrote:

Hiren,

Thanks for finding this bug and proposing a solution! Your solution looks fine in the sense that it mimics the rest of the behavior of this function when validation fails.

However, I think this function (and tcp_input(), which calls it) actually do the wrong thing. I believe the correct behavior should be:

• If we have a matching and valid syncache entry, setup the connection.
• If we do not find a matching connection, send a RST.

Which is what we are doing right now but tsecr being 0 happens to be a known bad case. And we are giving it a chance to build the connection. (As later we do expect ACKs with ecr0 and we just don't use them in RTT measurement and treat them as if connection doesn't have timestamps)

• If we find a matching syncache entry, but there is a problem with validating it (e.g. RST, etc.), ignore it.

Hum, can you please provide an example of this? what could 3rd maching ack bring that doesn't validate? I am probably missing something obvious.
I generally agree that RST would be too extreme in such cases.

The consequence of the current behavior is that an errant TCP packet can tear down a connection attempt.

Of course, I'm not sure my proposed behavior is much better. It has at least two problems:

• How do you apply these rules to syncookies?
• An errant SYN will prevent another connection from the same host/port until the syncache entry times out.

So, my proposed behavior is probably not fully correct, either.

In any case, I just wanted to take this opportunity to note that I think we should have a conversation about the correct behavior here.

Thanks a lot for your comments and I agree that such checks are harder to enforce in syncache/syncookie.

[hiren]

• If we find a matching syncache entry, but there is a problem with validating it (e.g. RST, etc.), ignore it.

Hum, can you please provide an example of this? what could 3rd maching ack bring that doesn't validate? I am probably missing something obvious.

Ah, I think you are talking about "Segment validation" in syncache_expand(). Any of them failing would cause tcp_input() to send RST with rstreason = BANDLIM_RST_OPENPORT and goto dropwithreset. And will delete the syncache entry also.