Only validate SEG.SEQ and SEG.ACK when processing a real SYN-cache entry. In the SYN-cookie case, these conditions are always true, since the SYN-cache entry on the stack is constructed from the incoming TCP segment.
While there, fix the logging messages.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This change looks good to me, though I personally prefer to the more verbose comments before you shortened them. But I wouldn't block on that.
One comment about the preexisting pattern here: requiring every failure path to individually unlock is error prone IMHO, as well as having a mix of return and goto. There's already a locked variable too. I don't think a single exit point with the cleanup would be too hard to add... just a thought.
Comment Actions
I agree with Nick that previous comments were better.
You may add assertions that match the checks in the syncookie_expand() case.
Also, I'm not sure "not a functional change" applies in the commit message. TCP wise there is no change, but code wise there is.
P.S. Interesting that the email generator of phabricator produced a much easier diff to read than the web version.
Comment Actions
OK. Moving the checks to the proper place is the first step. The second step will be to do the right action, which is sending a challenge ACK instead of killing the entry. When writing the comments for the second step, I will expand the variable names again.
One comment about the preexisting pattern here: requiring every failure path to individually unlock is error prone IMHO, as well as having a mix of return and goto. There's already a locked variable too. I don't think a single exit point with the cleanup would be too hard to add... just a thought.
Once we have the right functionality, we can cleanup the function. I just did not wanted to mix these things up.
Comment Actions
I can expand the variable names. But I would like to be consistent in stating what is tested: in one test SEG.SEQ, and in the other SEG.ACK.
You may add assertions that match the checks in the syncookie_expand() case.
I will take this into account when the code is doing the actions it should.
Also, I'm not sure "not a functional change" applies in the commit message. TCP wise there is no change, but code wise there is.
That is not there anymore...
P.S. Interesting that the email generator of phabricator produced a much easier diff to read than the web version.
I totally agree....