Improve DSACK computation
ClosedPublic
Actions

Authored by tuexen on Sep 8 2019, 8:31 PM.

Details

Reviewers

rscheff
rrs

Group Reviewers

transport

Commits

rS352284: MFC r352072:
rS352072: Only update SACK/DSACK lists when a non-empty segment was received.

Summary

The SACK/ DSACK lists only have to be updated, if a non-empty segment was received.
Without this check, when an empty FIN-segment is received which appends the FIN to the last gap, a KASSERT would be hit.

Test Plan

Here is a reproducer for the problem:

dsack-fin.pkt2 KBDownload

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 26348

Event Timeline

tuexen created this revision.Sep 8 2019, 8:31 PM

Herald added a reviewer: transport. · View Herald TranscriptSep 8 2019, 8:31 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B26341: Diff 61815.Sep 8 2019, 8:31 PM

I would like to see the Rack version of the code be identical to the tcp_input version.. i.e. it should check

if ((tlen > 0) && (tp->t_flags & TF_SACK_PERMITED))

The soon to be committed version of rack will allow a user to use the stack without sack.. so it is best to
put this in place now.

This revision now requires changes to proceed.Sep 9 2019, 9:44 AM

Add a check to the RACK code whether the SACK extension is enabled. This was requested by rrs@.

Harbormaster completed remote builds in B26348: Diff 61827.Sep 9 2019, 9:54 AM

In D21567#469962, @rrs wrote:

I would like to see the Rack version of the code be identical to the tcp_input version.. i.e. it should check

if ((tlen > 0) && (tp->t_flags & TF_SACK_PERMITED))

The soon to be committed version of rack will allow a user to use the stack without sack.. so it is best to
put this in place now.

Fixed by using if ((tp->t_flags & TF_SACK_PERMIT) && (save_tlen > 0)) { in both stacks.

Checked this patch with various "pure FIN after hole" and "data FIN after hole" combinations; the FIN bit becomes part of the SACK (and DSACK) blocks, as it should be when considering that FIN is part of the sequence number space.

However, the handling of SYN and FIN in SACK/DSACK is not explicitly stated in RFC2883 though.

sys/netinet/tcp_input.c
3049	you can remove the outermost bracket here.
3057	This comment is no longer valid now. missed this with the full rfc2883 update.

Address Richard's comments.

Harbormaster completed remote builds in B26350: Diff 61830.Sep 9 2019, 11:26 AM

The patch has been updated to address Richard's comments.

sys/netinet/tcp_input.c
3049	Done.
3057	Removed.

tuexen marked 2 inline comments as done.Sep 9 2019, 11:27 AM

rscheff accepted this revision.Sep 9 2019, 11:51 AM

In D21567#469977, @rscheff_gmx.at wrote:

Checked this patch with various "pure FIN after hole" and "data FIN after hole" combinations; the FIN bit becomes part of the SACK (and DSACK) blocks, as it should be when considering that FIN is part of the sequence number space.

I don't think the above is true with the code right now. But changing it that the FIN is dealt with in SACK/DSACK is a separate commit, I think,

However, the handling of SYN and FIN in SACK/DSACK is not explicitly stated in RFC2883 though.

In D21567#470074, @tuexen wrote:

In D21567#469977, @rscheff_gmx.at wrote:

Checked this patch with various "pure FIN after hole" and "data FIN after hole" combinations; the FIN bit becomes part of the SACK (and DSACK) blocks, as it should be when considering that FIN is part of the sequence number space.

I don't think the above is true with the code right now. But changing it that the FIN is dealt with in SACK/DSACK is a separate commit, I think,

Yes; I tested with pure FIN, FIN with data, and various overlaps. The Panic no longer happened in any of these combinations, but I have *not* observed the FIN bit to become part of a SACK or DSACK block (which triggered my request about the expected behavior; as SYN and FIN are part of the sequence number space, logically these "should" also become "SACK"able, even if nowhere explicitly stated. However, this may have some other consequences which definitely warrant a separate commit. e.g. Data-after FIN with holes, how does the reassembly code react)

rrs accepted this revision.Sep 9 2019, 3:16 PM

This revision is now accepted and ready to land.Sep 9 2019, 3:16 PM

Closed by commit rS352072: Only update SACK/DSACK lists when a non-empty segment was received. (authored by tuexen). · Explain WhySep 9 2019, 4:08 PM

This revision was automatically updated to reflect the committed changes.

tuexen added a commit: rS352072: Only update SACK/DSACK lists when a non-empty segment was received..

tuexen added a commit: rS352284: MFC r352072:.Sep 13 2019, 8:15 AM

tuexen mentioned this in rS360282: Improve the TCP SACK generation by reporting DSACKs.Apr 24 2020, 9:30 PM