tcp_output: Clear FIN if tcp_m_copym truncates output length

[jhb]

Summary

Sponsored by: Chelsio Communications

[jhb]

I tripped over this while doing some internal testing of NIC TLS where I used a hack in tcp_m_copym() that forces truncations in the middle of TLS trailers and I tripped over an assertion that a truncated TLS record can't be transmitted with FIN. Other places in tcp_output() that truncate the length clear TH_FIN as well. Possibly the change to clear tso could also go inside this check, but that would be a larger diff.

[rscheff]

Good find! While there is probably zero difference as the compiler optimizes this, keeping "old_len" instead would only require the clearing of TH_FIN, and not another assignment though, which seems more alike to similar other checks... @tuexen @cc and @rrs may want to have a look too...

[tuexen]

In D46824#1067840, @rscheff wrote:

Good find! While there is probably zero difference as the compiler optimizes this, keeping "old_len" instead would only require the clearing of TH_FIN, and not another assignment though, which seems more alike to similar other checks... @tuexen @cc and @rrs may want to have a look too...

I agree with Richard. Something like:

uint32_t old_len;
...
old_len = len;
m->m_next = tcp_m_copym(mb, moff,
    &len, if_hw_tsomaxsegcount,
    if_hw_tsomaxsegsize, msb, hw_tls);
if (old_len != len)
	 flags &= ~TH_FIN;

But I leave it up to you.

[jhb]

Switch to old_len

[rscheff]

[cc]

sys/netinet/tcp_output.c

1081

Shall use int32_t, the same type as len. Although we generally think uint32_t is not harmful, but I think we shall stick with the simple rule when comparing int32_t with uint32_t.

[cc]

I think this change also applies to the bbr and rack stacks.

[jhb]

I can fix the type mismatch during commit. I have not looked to see if other stacks are affected.

[tuexen]

In D46824#1068981, @jhb wrote:

I can fix the type mismatch during commit. I have not looked to see if other stacks are affected.

Fixing the type mismatch would be good. I think other stacks are not affected, since I think they
do not send a FIN before any outstanding data is ACKed and nothing is buffered anymore.

[cc]

In D46824#1068983, @tuexen wrote:

In D46824#1068981, @jhb wrote:

I can fix the type mismatch during commit. I have not looked to see if other stacks are affected.

Fixing the type mismatch would be good. I think other stacks are not affected, since I think they
do not send a FIN before any outstanding data is ACKed and nothing is buffered anymore.

Sounds good to me.