Improve DSACK computation

[tuexen]

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

[rrs]

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.

[tuexen]

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

[tuexen]

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.

[rscheff]

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.

3056–3057

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

[tuexen]

Address Richard's comments.

[tuexen]

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

sys/netinet/tcp_input.c

3049

Done.

3056–3057

Removed.

[tuexen]

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.

[rscheff]

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)