tcp: improve SEG.ACK validation in SYN-RECEIVED
ClosedPublic
Actions

Authored by tuexen on Mon, Oct 6, 2:42 PM.

Details

Reviewers

jtl
rscheff
rrs
cc
peter.lei_ieee.org
lstewart
glebius

Group Reviewers

transport

Commits

rG41e9c68d9ac1: tcp: improve SEG.ACK validation in SYN-RECEIVED
rG1728dae25358: tcp: improve SEG.ACK validation in SYN-RECEIVED
rG8af2f06a99b1: tcp: improve SEG.ACK validation in SYN-RECEIVED

Summary

According to the fifth step in SEGMENT ARRIVES, send a RST segment in response to an ACK segment which fails the SEG.ACK check, but leave the endpoint state unchanged.
FreeBSD handles this correctly when entering the SYN-RECEIVED state via the SYN-SENT state, but not in the SYN-cache code, which handles the SYN-RECEIVED state via the LISTEN state.
This also fixes a panic reported by Alexander Leidinger.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tuexen created this revision.Mon, Oct 6, 2:42 PM

Herald added a reviewer: transport. · View Herald TranscriptMon, Oct 6, 2:42 PM

Herald added subscribers: glebius, melifaro, imp. · View Herald Transcript

tuexen requested review of this revision.Mon, Oct 6, 2:42 PM

tuexen retitled this revision from tcp: improve SEG.ACK check in SYN-RECEIVED to tcp: improve SEG.ACK validation in SYN-RECEIVED.Mon, Oct 6, 2:48 PM

jtl accepted this revision.Mon, Oct 6, 3:25 PM

jtl added inline comments.

sys/netinet/tcp_syncache.c
1287	Is there a reason to free() while holding the lock? The conditional right before this one drops the lock before calling free().
1318	At this point, I think you can either delete this or turn it into an assert that sc == &scs. (I think you can now only reach this from the syncookie portion of the code and not from the syncache code.)

This revision is now accepted and ready to land.Mon, Oct 6, 3:25 PM

Address one of Jonathan's comments.

This revision now requires review to proceed.Mon, Oct 6, 3:53 PM

tuexen added inline comments.Mon, Oct 6, 3:54 PM

sys/netinet/tcp_syncache.c
1287	This is one of the things I want to do consistently. In several other places it calls free in the `if` statement, at the end it checks if `s` is not `NULL`. I plan to do it in a consistent way in all places. Doing it after releasing the lock is better, I agree. I can do that right now.
1318	Yes, this is also one of the cleanup steps. Right now my plan is to remove the `synchache_free()` call, move the `free(s, M_TCPLOG)` up to the remaining three places and finally remove the three instances of `goto failed` by using `return (0)` directly. I guess this makes the code clearer.

glebius accepted this revision.Mon, Oct 6, 6:24 PM

This revision is now accepted and ready to land.Mon, Oct 6, 6:24 PM

Closed by commit rG8af2f06a99b1: tcp: improve SEG.ACK validation in SYN-RECEIVED (authored by tuexen). · Explain WhyMon, Oct 6, 8:43 PM

This revision was automatically updated to reflect the committed changes.

tuexen added a commit: rG8af2f06a99b1: tcp: improve SEG.ACK validation in SYN-RECEIVED.

I can confirm that this fixes the crash I've seen. Instead of crashing after a few minutes, it now is still humming happily with 16 minutes of uptime.

sys/netinet/tcp_syncache.c
1289	';' in front and after the comment...

In D52934#1209636, @netchild wrote:

I can confirm that this fixes the crash I've seen. Instead of crashing after a few minutes, it now is still humming happily with 16 minutes of uptime.

Thanks for testing and reporting.