Open matttbe opened 2 days ago
nit: RFC says we "SHOULD" use middlebox interference (MPTCP_RST_EMIDDLEBOX
or 0x6 ) in this scenario, not the generic one (that's 0x1). currently get_mapping_status()
returns MAPPING_INVALID
. we might want to use a dedicated value of mapping_status for situations:
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -960,7 +960,8 @@ enum mapping_status {
MAPPING_EMPTY,
MAPPING_DATA_FIN,
MAPPING_DUMMY,
- MAPPING_BAD_CSUM
+ MAPPING_BAD_CSUM,
+ MAPPING_NODSS
};
static void dbg_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
@@ -1103,6 +1104,10 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
mpext = mptcp_get_ext(skb);
if (!mpext || !mpext->use_map) {
+
+ if (!mpext)
+ return MAPPING_NODSS;
+
if (!subflow->map_valid && !skb->len) {
/* the TCP stack deliver 0 len FIN pkt to the receive
* queue, that is the only 0len pkts ever expected here,
@ -1332,7 +1337,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
status = get_mapping_status(ssk, msk);
trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
if (unlikely(status == MAPPING_INVALID || status == MAPPING_DUMMY ||
- status == MAPPING_BAD_CSUM))
+ status == MAPPING_BAD_CSUM || status == MAPPING_NODSS))
goto fallback;
if (status != MAPPING_OK)
@@ -1385,7 +1390,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
* subflow_error_report() will introduce the appropriate barriers
*/
subflow->reset_transient = 0;
- subflow->reset_reason = MPTCP_RST_EMPTCP;
+ subflow->reset_reason = status == MAPPING_NODSS ? MPTCP_RST_EMIDDLEBOX : MPTCP_RST_EMPTCP;
Note that removing these two lines... https://github.com/multipath-tcp/mptcp_net-next/blob/ecea735d56097b4e631fa4a89b3bdf42160289f4/net/mptcp/protocol.c#L3361-L3362
...makes this test fails:
v1_mp_capable_bind_no_cs_ooo.pkt:16: error handling packet: live packet field ipv6_payload_len: expected: 40 (0x28) vs actual: 32 (0x20)
script packet: 0.013904 . 1:1(0) ack 1 <nop,nop,sack 101:201,dss dack4 16777216 flags: A>
actual packet: 0.013883 . 1:1(0) ack 1 win 1050 <nop,nop,sack 101:201>
Should it do a fallback?
Also, the packetdrill I shared doesn't pass with this patch.
Also, the packetdrill I shared doesn't pass with this patch.
I gave a quick look. The whole 'fully_established' tracking will require a more significant refactor to match accurately the RFC spec; check_fully_established() being the main curprit. Such function will need to track more status and look at more pkts.
TL;DR: this path does not look suitable for small fix. I suggest we rollback to the previous idea, using ->allow_infinite_fallback.
We can plan the fully_establish refactor for a later net-next series.
As a side effect, no new squash-to patches for "mptcp: handle consistently DSS corruption"
@cpaasch reported me an issue seen in the field, due to a middlebox stripping MPTCP options later on after the 3WHS.
This packetdrill test is what we are supposed to have:
But instead of sending a "plain TCP" ACK for the last packet here, we can see a RST packet with MP_TCPRST set to 1:
The server sends a RST, because when it detects the errors, it first checks if a fallback is possible [1] using
subflow_can_fallback()
which returnsfalse
due to:Technically, the server should not be in
fully_established
mode here, because it didn't receive a validDATA_ACK
from the client, nor data for more than the initial window (implying aDATA_ACK
has been received from the other side).But not to change the whole behaviour, maybe easier to do something like this if it is OK with the RFC: