| 1 | This series fixes the issue #544 where a middlebox drops MPTCP options | 1 | - Patch 1: fix issue #544 where a middlebox drops MPTCP options after a |
|---|---|---|---|
| 2 | after a 3WHS on the additional subflow. | 2 | 3WHS on the additional subflow. |
| 3 | 3 | ||
| 4 | Two additional patches have been added: | 4 | - Patch 2: a safety check is added when asking to do a fallback to do so |
| 5 | 5 | only when allowed. If not, a new warning will be emitted. | |
| 6 | - Patch 2: for an issue seen when debugging the previous one: there | ||
| 7 | should be a fallback after the reception of an infinite mapping, only | ||
| 8 | if allowed. | ||
| 9 | |||
| 10 | - Patch 3: to avoid the issues fixed by the two patches above, a safety | ||
| 11 | check is added when asking to do a fallback if it is not possible to | ||
| 12 | do so. A warning will be emitted in this case. | ||
| 13 | 6 | ||
| 14 | Note that two new packetdrill tests have been added in a PR to cover | 7 | Note that two new packetdrill tests have been added in a PR to cover |
| 15 | this case: | 8 | this case: |
| 16 | 9 | ||
| 17 | https://github.com/multipath-tcp/packetdrill/pull/160 | 10 | https://github.com/multipath-tcp/packetdrill/pull/160 |
| 18 | 11 | ||
| 19 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> | 12 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> |
| 20 | --- | 13 | --- |
| 21 | Matthieu Baerts (NGI0) (3): | 14 | Changes in v2: |
| 15 | - Squashed previous patches 1 and 2 (Paolo). | ||
| 16 | - Patch 1: no more special cases for csum (handled before) and inf map | ||
| 17 | (Paolo). | ||
| 18 | - Link to v1: https://lore.kernel.org/r/20250217-mptcp-dss-drop-mpj-v1-0-d671d6b9a153@kernel.org | ||
| 19 | |||
| 20 | --- | ||
| 21 | Matthieu Baerts (NGI0) (2): | ||
| 22 | mptcp: reset when MPTCP opts are dropped after join | 22 | mptcp: reset when MPTCP opts are dropped after join |
| 23 | mptcp: only fallback due to inf mapping if allowed | ||
| 24 | mptcp: safety check before fallback | 23 | mptcp: safety check before fallback |
| 25 | 24 | ||
| 26 | net/mptcp/protocol.h | 2 ++ | 25 | net/mptcp/protocol.h | 2 ++ |
| 27 | net/mptcp/subflow.c | 23 ++++++++++++++--------- | 26 | net/mptcp/subflow.c | 15 +-------------- |
| 28 | 2 files changed, 16 insertions(+), 9 deletions(-) | 27 | 2 files changed, 3 insertions(+), 14 deletions(-) |
| 29 | --- | 28 | --- |
| 30 | base-commit: 3a7786db65e8f361a8fc8abcd3cfbe17411b8b7f | 29 | base-commit: 3a7786db65e8f361a8fc8abcd3cfbe17411b8b7f |
| 31 | change-id: 20250205-mptcp-dss-drop-mpj-ec227f4ed942 | 30 | change-id: 20250205-mptcp-dss-drop-mpj-ec227f4ed942 |
| 32 | 31 | ||
| 33 | Best regards, | 32 | Best regards, |
| 34 | -- | 33 | -- |
| 35 | Matthieu Baerts (NGI0) <matttbe@kernel.org> | 34 | Matthieu Baerts (NGI0) <matttbe@kernel.org> | diff view generated by jsdifflib |
| ... | ... | ||
|---|---|---|---|
| 3 | invalid mapping, map_data_len was not set to the data len, and then the | 3 | invalid mapping, map_data_len was not set to the data len, and then the |
| 4 | subflow was not reset as it should have been, leaving the MPTCP | 4 | subflow was not reset as it should have been, leaving the MPTCP |
| 5 | connection in a wrong fallback mode. | 5 | connection in a wrong fallback mode. |
| 6 | 6 | ||
| 7 | This map_data_len condition has been introduced to handle the reception | 7 | This map_data_len condition has been introduced to handle the reception |
| 8 | of the infinite mapping. So instead, a new dedicated mapping error is | 8 | of the infinite mapping. Instead, a new dedicated mapping error could |
| 9 | now returned (infinite), and this special case is properly handled: the | 9 | have been returned and treated as a special case. However, the commit |
| 10 | exception is only applied to this case now, and not other ones by | 10 | 31bf11de146c ("mptcp: introduce MAPPING_BAD_CSUM") has been introduced |
| 11 | mistake. | 11 | by Paolo Abeni soon after, and backported later on to stable. It better |
| 12 | handle the csum case, and it means the exception for valid_csum_seen in | ||
| 13 | subflow_can_fallback(), plus this one for the infinite mapping in | ||
| 14 | subflow_check_data_avail(), are no longer needed. | ||
| 12 | 15 | ||
| 13 | While at it, no need to set map_data_len to 0 as it will be set to | 16 | In other words, the code can be simplified there: a fallback should only |
| 14 | skb->len just after, at the end of subflow_check_data_avail(). | 17 | be done if msk->allow_infinite_fallback is set. This boolean is set to |
| 18 | false once MPTCP-specific operations acting on the whole MPTCP | ||
| 19 | connection vs the initial path have been done, e.g. a second path has | ||
| 20 | been created, or an MPTCP re-injection -- yes, possible even with a | ||
| 21 | single subflow. The subflow_can_fallback() helper can then be dropped, | ||
| 22 | and replaced by this single condition. | ||
| 23 | |||
| 24 | This also makes the code clearer: a fallback should only be done if it | ||
| 25 | is possible to do so. | ||
| 26 | |||
| 27 | While at it, no need to set map_data_len to 0 in get_mapping_status() | ||
| 28 | for the infinite mapping case: it will be set to skb->len just after, at | ||
| 29 | the end of subflow_check_data_avail(), and not read in between. | ||
| 15 | 30 | ||
| 16 | Fixes: f8d4bcacff3b ("mptcp: infinite mapping receiving") | 31 | Fixes: f8d4bcacff3b ("mptcp: infinite mapping receiving") |
| 17 | Reported-by: Chester A. Unal <chester.a.unal@xpedite-tech.com> | 32 | Reported-by: Chester A. Unal <chester.a.unal@xpedite-tech.com> |
| 18 | Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/544 | 33 | Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/544 |
| 19 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> | 34 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> |
| 20 | --- | 35 | --- |
| 21 | net/mptcp/subflow.c | 9 +++++---- | 36 | Notes: |
| 22 | 1 file changed, 5 insertions(+), 4 deletions(-) | 37 | - v2: |
| 38 | - Squash previous patches 1 and 2, and drop csum case from | ||
| 39 | subflow_can_fallback (Paolo). | ||
| 40 | - v2 is quite different, Chester's Tested-by tag has not been added. | ||
| 41 | --- | ||
| 42 | net/mptcp/subflow.c | 15 +-------------- | ||
| 43 | 1 file changed, 1 insertion(+), 14 deletions(-) | ||
| 23 | 44 | ||
| 24 | diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c | 45 | diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c |
| 25 | index XXXXXXX..XXXXXXX 100644 | 46 | index XXXXXXX..XXXXXXX 100644 |
| 26 | --- a/net/mptcp/subflow.c | 47 | --- a/net/mptcp/subflow.c |
| 27 | +++ b/net/mptcp/subflow.c | 48 | +++ b/net/mptcp/subflow.c |
| 28 | @@ -XXX,XX +XXX,XX @@ enum mapping_status { | ||
| 29 | MAPPING_DATA_FIN, | ||
| 30 | MAPPING_DUMMY, | ||
| 31 | MAPPING_BAD_CSUM, | ||
| 32 | + MAPPING_INFINITE, | ||
| 33 | MAPPING_NODSS | ||
| 34 | }; | ||
| 35 | |||
| 36 | @@ -XXX,XX +XXX,XX @@ static enum mapping_status get_mapping_status(struct sock *ssk, | 49 | @@ -XXX,XX +XXX,XX @@ static enum mapping_status get_mapping_status(struct sock *ssk, |
| 37 | if (data_len == 0) { | 50 | if (data_len == 0) { |
| 38 | pr_debug("infinite mapping received\n"); | 51 | pr_debug("infinite mapping received\n"); |
| 39 | MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX); | 52 | MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX); |
| 40 | - subflow->map_data_len = 0; | 53 | - subflow->map_data_len = 0; |
| 41 | - return MAPPING_INVALID; | 54 | return MAPPING_INVALID; |
| 42 | + return MAPPING_INFINITE; | ||
| 43 | } | 55 | } |
| 44 | 56 | ||
| 45 | if (mpext->data_fin == 1) { | 57 | @@ -XXX,XX +XXX,XX @@ static void subflow_sched_work_if_closed(struct mptcp_sock *msk, struct sock *ss |
| 46 | @@ -XXX,XX +XXX,XX @@ static bool subflow_check_data_avail(struct sock *ssk) | 58 | mptcp_schedule_work(sk); |
| 47 | status = get_mapping_status(ssk, msk); | 59 | } |
| 48 | trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue)); | 60 | |
| 49 | if (unlikely(status == MAPPING_INVALID || status == MAPPING_DUMMY || | 61 | -static bool subflow_can_fallback(struct mptcp_subflow_context *subflow) |
| 50 | - status == MAPPING_BAD_CSUM || status == MAPPING_NODSS)) | 62 | -{ |
| 51 | + status == MAPPING_BAD_CSUM || status == MAPPING_INFINITE || | 63 | - struct mptcp_sock *msk = mptcp_sk(subflow->conn); |
| 52 | + status == MAPPING_NODSS)) | 64 | - |
| 53 | goto fallback; | 65 | - if (subflow->mp_join) |
| 54 | 66 | - return false; | |
| 55 | if (status != MAPPING_OK) | 67 | - else if (READ_ONCE(msk->csum_enabled)) |
| 68 | - return !subflow->valid_csum_seen; | ||
| 69 | - else | ||
| 70 | - return READ_ONCE(msk->allow_infinite_fallback); | ||
| 71 | -} | ||
| 72 | - | ||
| 73 | static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk) | ||
| 74 | { | ||
| 75 | struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk); | ||
| 56 | @@ -XXX,XX +XXX,XX @@ static bool subflow_check_data_avail(struct sock *ssk) | 76 | @@ -XXX,XX +XXX,XX @@ static bool subflow_check_data_avail(struct sock *ssk) |
| 57 | return true; | 77 | return true; |
| 58 | } | 78 | } |
| 59 | 79 | ||
| 60 | - if (!subflow_can_fallback(subflow) && subflow->map_data_len) { | 80 | - if (!subflow_can_fallback(subflow) && subflow->map_data_len) { |
| 61 | + if (!subflow_can_fallback(subflow) && status != MAPPING_INFINITE) { | 81 | + if (!READ_ONCE(msk->allow_infinite_fallback)) { |
| 62 | /* fatal protocol error, close the socket. | 82 | /* fatal protocol error, close the socket. |
| 63 | * subflow_error_report() will introduce the appropriate barriers | 83 | * subflow_error_report() will introduce the appropriate barriers |
| 64 | */ | 84 | */ |
| 65 | 85 | ||
| 66 | -- | 86 | -- |
| 67 | 2.47.1 | 87 | 2.47.1 | diff view generated by jsdifflib |
Deleted patch | |||
|---|---|---|---|
| 1 | Any fallback should happen only if allowed, so only if this variable is | ||
| 2 | set: msk->allow_infinite_fallback. This boolean will be set to false | ||
| 3 | once MPTCP-specific operations acting on the whole MPTCP connection vs | ||
| 4 | the initial path have been done, e.g. a second path has been created, or | ||
| 5 | an MPTCP re-injection -- yes, possible even with a single subflow. | ||
| 6 | 1 | ||
| 7 | In other words, the !msk->allow_infinite_fallback condition should be | ||
| 8 | placed first. If it is no longer possible to do a fallback, there should | ||
| 9 | not be any bypass, e.g. when receiving an infinite mapping. | ||
| 10 | |||
| 11 | Now, regarding the conditions to do a fallback, the RFC mentions [1] | ||
| 12 | that if the checksum is used, a fallback is only possible when "it is | ||
| 13 | known that all unacknowledged data in flight is contiguous (which will | ||
| 14 | usually be the case with a single subflow)". In other words, if the | ||
| 15 | checksum is used, a fallback is possible when the other peer sent an | ||
| 16 | infinite mapping indicating the flow has been altered. | ||
| 17 | |||
| 18 | Note that the issue is present since a merge commit, where both | ||
| 19 | subflow_can_fallback() and the previous extra condition with | ||
| 20 | 'subflow->map_data_len' got introduced. | ||
| 21 | |||
| 22 | Fixes: d7e6f5836038 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net") | ||
| 23 | Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.7-11 [1] | ||
| 24 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> | ||
| 25 | --- | ||
| 26 | net/mptcp/subflow.c | 16 ++++++++++------ | ||
| 27 | 1 file changed, 10 insertions(+), 6 deletions(-) | ||
| 28 | |||
| 29 | diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c | ||
| 30 | index XXXXXXX..XXXXXXX 100644 | ||
| 31 | --- a/net/mptcp/subflow.c | ||
| 32 | +++ b/net/mptcp/subflow.c | ||
| 33 | @@ -XXX,XX +XXX,XX @@ static void subflow_sched_work_if_closed(struct mptcp_sock *msk, struct sock *ss | ||
| 34 | mptcp_schedule_work(sk); | ||
| 35 | } | ||
| 36 | |||
| 37 | -static bool subflow_can_fallback(struct mptcp_subflow_context *subflow) | ||
| 38 | +static bool subflow_can_fallback(struct mptcp_subflow_context *subflow, | ||
| 39 | + enum mapping_status status) | ||
| 40 | { | ||
| 41 | struct mptcp_sock *msk = mptcp_sk(subflow->conn); | ||
| 42 | |||
| 43 | - if (subflow->mp_join) | ||
| 44 | + /* If not allowed (additional paths, MPTCP reinjections): no fallback */ | ||
| 45 | + if (!READ_ONCE(msk->allow_infinite_fallback)) | ||
| 46 | return false; | ||
| 47 | - else if (READ_ONCE(msk->csum_enabled)) | ||
| 48 | + | ||
| 49 | + /* More strict with csum: fallback in 2 cases: inf map or never valid */ | ||
| 50 | + if (status != MAPPING_INFINITE && READ_ONCE(msk->csum_enabled)) | ||
| 51 | return !subflow->valid_csum_seen; | ||
| 52 | - else | ||
| 53 | - return READ_ONCE(msk->allow_infinite_fallback); | ||
| 54 | + | ||
| 55 | + return true; | ||
| 56 | } | ||
| 57 | |||
| 58 | static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk) | ||
| 59 | @@ -XXX,XX +XXX,XX @@ static bool subflow_check_data_avail(struct sock *ssk) | ||
| 60 | return true; | ||
| 61 | } | ||
| 62 | |||
| 63 | - if (!subflow_can_fallback(subflow) && status != MAPPING_INFINITE) { | ||
| 64 | + if (!subflow_can_fallback(subflow, status)) { | ||
| 65 | /* fatal protocol error, close the socket. | ||
| 66 | * subflow_error_report() will introduce the appropriate barriers | ||
| 67 | */ | ||
| 68 | |||
| 69 | -- | ||
| 70 | 2.47.1 | diff view generated by jsdifflib |
| 1 | Recently, some fallback have been initiated, while the connection was | 1 | Recently, some fallback have been initiated, while the connection was |
|---|---|---|---|
| 2 | not supposed to fallback. | 2 | not supposed to fallback. |
| 3 | 3 | ||
| 4 | Add a safety check with a warning to detect when an wrong attempt to | 4 | Add a safety check with a warning to detect when an wrong attempt to |
| 5 | fallback is being done. This should help detecting any future issues | 5 | fallback is being done. This should help detecting any future issues |
| 6 | quicker. | 6 | quicker. |
| 7 | 7 | ||
| 8 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> | 8 | Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> |
| 9 | --- | 9 | --- |
| 10 | Notes: | 10 | Notes: |
| 11 | - I guess this patch could be sent to -net as well. | 11 | - I guess this patch could be sent to -net as well. |
| 12 | - We could also squash this in one of the previous commit, but because | 12 | - We could also squash this in one of the previous commit, but because |
| 13 | the two are needed to avoid the warning, maybe better with a | 13 | the two are needed to avoid the warning, maybe better with a |
| 14 | dedicated commit? | 14 | dedicated commit? |
| 15 | --- | 15 | --- |
| 16 | net/mptcp/protocol.h | 2 ++ | 16 | net/mptcp/protocol.h | 2 ++ |
| 17 | 1 file changed, 2 insertions(+) | 17 | 1 file changed, 2 insertions(+) |
| 18 | 18 | ||
| 19 | diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h | 19 | diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h |
| 20 | index XXXXXXX..XXXXXXX 100644 | 20 | index XXXXXXX..XXXXXXX 100644 |
| 21 | --- a/net/mptcp/protocol.h | 21 | --- a/net/mptcp/protocol.h |
| 22 | +++ b/net/mptcp/protocol.h | 22 | +++ b/net/mptcp/protocol.h |
| 23 | @@ -XXX,XX +XXX,XX @@ static inline void __mptcp_do_fallback(struct mptcp_sock *msk) | 23 | @@ -XXX,XX +XXX,XX @@ static inline void __mptcp_do_fallback(struct mptcp_sock *msk) |
| 24 | pr_debug("TCP fallback already done (msk=%p)\n", msk); | 24 | pr_debug("TCP fallback already done (msk=%p)\n", msk); |
| 25 | return; | 25 | return; |
| 26 | } | 26 | } |
| 27 | + if (WARN_ON_ONCE(!READ_ONCE(msk->allow_infinite_fallback))) | 27 | + if (WARN_ON_ONCE(!READ_ONCE(msk->allow_infinite_fallback))) |
| 28 | + return; | 28 | + return; |
| 29 | set_bit(MPTCP_FALLBACK_DONE, &msk->flags); | 29 | set_bit(MPTCP_FALLBACK_DONE, &msk->flags); |
| 30 | } | 30 | } |
| 31 | 31 | ||
| 32 | 32 | ||
| 33 | -- | 33 | -- |
| 34 | 2.47.1 | 34 | 2.47.1 | diff view generated by jsdifflib |