Page MenuHomeFreeBSD

Rack crash that Larry Rosenman found..
ClosedPublic

Authored by rrs on Jun 11 2018, 12:39 PM.

Details

Summary

Turns out Rack is not fully TFO capable (which we knew).. it branched before
TFO settled into the tree. Now this set of changes updates rack's TFO handling
to match the default stack. It may not be 100% right but hopefully no more crashes
will occur ;)

What this does is

  1. Bring in updates from tcp_input/output to rack that were done and missed in rack.
  2. Change some of those to understand the "rack-isms" on how rack does things so that we won't for example calculate a -1 offset in the socket buffer.
Test Plan

I have handed Larry a patch to re-test to see if it resolves his issue. Besides
the review I will wait for FeedBack from Larry on this.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rrs created this revision.Jun 11 2018, 12:39 PM
ler added a subscriber: ler.

I've got this and D15757 running since:
system boot Jun 11 08:24

We'll see

jtl added a subscriber: jtl.Jun 11 2018, 2:49 PM

This passes my "sniff test", but it would be better to get @pkelsey to review it.

kbowling added a subscriber: kbowling.
rrs updated this revision to Diff 43627.Jun 11 2018, 11:24 PM
rrs updated this revision to Diff 43637.Jun 12 2018, 5:58 AM

So this is weird we look like we sent a TFO and then hit a retransmit.. but somehow
the TFO flags was removed so we did not set len == 0 on having a SYN retransmitted.

Now that should just not be.. so I will now add a safety that checks for
Non_TFO and SYN .. and if its true it will zap len to 0.

This may not fly with TFO itself but I don't see how it hurts since if
we are not doing TFO you should never send a SYN with data.

rrs updated this revision to Diff 43649.Jun 12 2018, 11:59 AM

Turns out after looking at Larry's last core and contemplating it a bit if it
would have succeeded (which it did not) we would have crashed anyway.
This is because the SYN got included in the send_map and we would have
tried to trim 14 instead of the 13 that bytes in the socketbuffer. Fixing that
means the send_map cannot start at snd_una for the TFO case. This means
a bit more special code needs also to be added to the SYN_SENT case then
so you end up incrementing snd_una when you move to established and that
way when you call the ack processing code all will be well :)

ler added a comment.Jun 12 2018, 4:32 PM

New kernel with this running since:
system boot Jun 12 10:28

(US/CDT, UTC-5).

This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2018, 3:27 AM
This revision was automatically updated to reflect the committed changes.