From 0ca62e3752149ad37f73bf074db0a5f8fcaf6585 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Tue, 17 Aug 2021 20:50:35 +0200 Subject: [PATCH] dcsctp: Avoid bundling FORWARD-TSN and DATA chunks dcSCTP seems to be able to provoke usrsctp to send ABORT in some situations, as described in https://github.com/sctplab/usrsctp/issues/597. Using a packetdrill script, it seems as a contributing factor to this behavior is when a FORWARD-TSN chunk is bundled with a DATA chunk. This is a valid and recommended pattern in RFC3758: "F2) The data sender SHOULD always attempt to bundle an outgoing FORWARD TSN with outbound DATA chunks for efficiency." However, that seems to be a rare event in usrsctp, which generally sends each FORWARD-TSN in a separate packet. By doing the same, the assumption is that this scenario will generally be avoided. With many browsers and other clients using usrsctp, and which will not be upgraded for a long time, there is an advantage of avoiding the issue even if it is according to specification. Before this change, a FORWARD-TSN was bundled with outgoing DATA and due to this, it wasn't rate limited as the overhead was very little. With this change, a rate limiting behavior has been added to avoid sending too many FORWARD-TSN in small packets. It will be sent every RTT, or 200 ms, whichever is smallest. This is also described in the RFC. Bug: webrtc:12961 Change-Id: I3d8036a34f999f405958982534bfa0e99e330da3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229101 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/master@{#34801} --- net/dcsctp/socket/dcsctp_socket_test.cc | 82 +++++++++++++++++++ .../socket/transmission_control_block.cc | 30 +++++-- .../socket/transmission_control_block.h | 5 ++ 3 files changed, 110 insertions(+), 7 deletions(-) diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index 6e0bbf7b56..1c8525a6f3 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -26,6 +26,7 @@ #include "net/dcsctp/packet/chunk/data_chunk.h" #include "net/dcsctp/packet/chunk/data_common.h" #include "net/dcsctp/packet/chunk/error_chunk.h" +#include "net/dcsctp/packet/chunk/forward_tsn_chunk.h" #include "net/dcsctp/packet/chunk/heartbeat_ack_chunk.h" #include "net/dcsctp/packet/chunk/heartbeat_request_chunk.h" #include "net/dcsctp/packet/chunk/idata_chunk.h" @@ -1764,5 +1765,86 @@ TEST_F(DcSctpSocketTest, SendsOnlyLargePackets) { } } +TEST_F(DcSctpSocketTest, DoesntBundleForwardTsnWithData) { + ConnectSockets(); + + // Force an RTT measurement using heartbeats. + AdvanceTime(options_.heartbeat_interval); + RunTimers(); + + // HEARTBEAT + std::vector hb_req_a = cb_a_.ConsumeSentPacket(); + std::vector hb_req_z = cb_z_.ConsumeSentPacket(); + + constexpr DurationMs kRtt = DurationMs(80); + AdvanceTime(kRtt); + sock_z_.ReceivePacket(hb_req_a); + sock_a_.ReceivePacket(hb_req_z); + + // HEARTBEAT_ACK + sock_a_.ReceivePacket(cb_z_.ConsumeSentPacket()); + sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket()); + + SendOptions send_options; + send_options.max_retransmissions = 0; + std::vector payload(options_.mtu - 100); + + // Send an initial message that is received, but the SACK was lost + sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); + // DATA + sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket()); + // SACK (lost) + std::vector sack = cb_z_.ConsumeSentPacket(); + + // Queue enough messages to fill the congestion window. + do { + sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); + } while (!cb_a_.ConsumeSentPacket().empty()); + + // Enqueue at least one more. + sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); + + // Let all of them expire by T3-RTX and inspect what's sent. + AdvanceTime(options_.rto_initial); + RunTimers(); + + std::vector sent1 = cb_a_.ConsumeSentPacket(); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet1, SctpPacket::Parse(sent1)); + + EXPECT_THAT(packet1.descriptors(), SizeIs(1)); + EXPECT_EQ(packet1.descriptors()[0].type, ForwardTsnChunk::kType); + + std::vector sent2 = cb_a_.ConsumeSentPacket(); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet2, SctpPacket::Parse(sent2)); + + EXPECT_GE(packet2.descriptors().size(), 1u); + EXPECT_EQ(packet2.descriptors()[0].type, DataChunk::kType); + + // Drop all remaining packets that A has sent. + while (!cb_a_.ConsumeSentPacket().empty()) { + } + + // Replay the SACK, and see if a FORWARD-TSN is sent again. + sock_a_.ReceivePacket(sack); + + // It shouldn't be sent as not enough time has passed yet. Instead, more + // DATA chunks are sent, that are in the queue. + std::vector sent3 = cb_a_.ConsumeSentPacket(); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet3, SctpPacket::Parse(sent3)); + + EXPECT_GE(packet2.descriptors().size(), 1u); + EXPECT_EQ(packet3.descriptors()[0].type, DataChunk::kType); + + // Now let RTT time pass, to allow a FORWARD-TSN to be sent again. + AdvanceTime(kRtt); + sock_a_.ReceivePacket(sack); + + std::vector sent4 = cb_a_.ConsumeSentPacket(); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet4, SctpPacket::Parse(sent4)); + + EXPECT_THAT(packet4.descriptors(), SizeIs(1)); + EXPECT_EQ(packet4.descriptors()[0].type, ForwardTsnChunk::kType); +} + } // namespace } // namespace dcsctp diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc index cc29ebd67e..f0f1ab9782 100644 --- a/net/dcsctp/socket/transmission_control_block.cc +++ b/net/dcsctp/socket/transmission_control_block.cc @@ -82,8 +82,31 @@ void TransmissionControlBlock::MaybeSendSack() { } } +void TransmissionControlBlock::MaybeSendForwardTsn(SctpPacket::Builder& builder, + TimeMs now) { + if (now >= limit_forward_tsn_until_ && + retransmission_queue_.ShouldSendForwardTsn(now)) { + if (capabilities_.message_interleaving) { + builder.Add(retransmission_queue_.CreateIForwardTsn()); + } else { + builder.Add(retransmission_queue_.CreateForwardTsn()); + } + packet_sender_.Send(builder); + // https://datatracker.ietf.org/doc/html/rfc3758 + // "IMPLEMENTATION NOTE: An implementation may wish to limit the number of + // duplicate FORWARD TSN chunks it sends by ... waiting a full RTT before + // sending a duplicate FORWARD TSN." + // "Any delay applied to the sending of FORWARD TSN chunk SHOULD NOT exceed + // 200ms and MUST NOT exceed 500ms". + limit_forward_tsn_until_ = now + std::min(DurationMs(200), rto_.srtt()); + } +} + void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder, TimeMs now) { + // FORWARD-TSNs are sent as separate packets to avoid bugs.webrtc.org/12961. + MaybeSendForwardTsn(builder, now); + for (int packet_idx = 0; packet_idx < options_.max_burst && retransmission_queue_.can_send_data(); ++packet_idx) { @@ -108,13 +131,6 @@ void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder, builder.Add(data_tracker_.CreateSelectiveAck( reassembly_queue_.remaining_bytes())); } - if (retransmission_queue_.ShouldSendForwardTsn(now)) { - if (capabilities_.message_interleaving) { - builder.Add(retransmission_queue_.CreateIForwardTsn()); - } else { - builder.Add(retransmission_queue_.CreateForwardTsn()); - } - } absl::optional reconfig = stream_reset_handler_.MakeStreamResetRequest(); if (reconfig.has_value()) { diff --git a/net/dcsctp/socket/transmission_control_block.h b/net/dcsctp/socket/transmission_control_block.h index b189ae82e0..4510d83365 100644 --- a/net/dcsctp/socket/transmission_control_block.h +++ b/net/dcsctp/socket/transmission_control_block.h @@ -152,6 +152,9 @@ class TransmissionControlBlock : public Context { // Sends a SACK, if there is a need to. void MaybeSendSack(); + // Sends a FORWARD-TSN, if it is needed and allowed (rate-limited). + void MaybeSendForwardTsn(SctpPacket::Builder& builder, TimeMs now); + // Will be set while the socket is in kCookieEcho state. In this state, there // can only be a single packet outstanding, and it must contain the COOKIE // ECHO chunk as the first chunk in that packet, until the COOKIE ACK has been @@ -206,6 +209,8 @@ class TransmissionControlBlock : public Context { const TieTag tie_tag_; const std::function is_connection_established_; PacketSender& packet_sender_; + // Rate limiting of FORWARD-TSN. Next can be sent at or after this timestamp. + TimeMs limit_forward_tsn_until_ = TimeMs(0); RetransmissionTimeout rto_; RetransmissionErrorCounter tx_error_counter_;