Page MenuHomeFreeBSD

Improve DSACK computation
ClosedPublic

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

Details

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:

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

tuexen created this revision.Sun, Sep 8, 8:31 PM
rrs requested changes to this revision.Mon, Sep 9, 9:44 AM

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.Mon, Sep 9, 9:44 AM
tuexen updated this revision to Diff 61827.Mon, Sep 9, 9:53 AM

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

tuexen added a comment.Mon, Sep 9, 9:55 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 ↗(On Diff #61815)

you can remove the outermost bracket here.

3057 ↗(On Diff #61815)

This comment is no longer valid now. missed this with the full rfc2883 update.

tuexen updated this revision to Diff 61830.Mon, Sep 9, 11:26 AM

Address Richard's comments.

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

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

sys/netinet/tcp_input.c
3049 ↗(On Diff #61815)

Done.

3057 ↗(On Diff #61815)

Removed.

tuexen marked 2 inline comments as done.Mon, Sep 9, 11:27 AM
rscheff_gmx.at accepted this revision.Mon, Sep 9, 11:51 AM
tuexen added a comment.Mon, Sep 9, 2:25 PM

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.

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.Mon, Sep 9, 3:16 PM
This revision is now accepted and ready to land.Mon, Sep 9, 3:16 PM