| Message ID | 4b2cee91613e597a172b46bb0d9d3143053c52da.1728408247.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 59b3ca4d902cd214a2381bc70149e81d3f41945b |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-next] mptcp: use "middlebox interference" for MP_TCPRST in case map_valid is false | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 39 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi Davide,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Critical: Global Timeout ❌
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11240855539
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/184902b101dc
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=896827
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Davide,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11240855539
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/184902b101dc
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=896827
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Davide, On 08/10/2024 19:31, Davide Caratti wrote: > RFC8684 suggests use of "Middlebox interference (code 0x06)" in case of > MPJ subflow that carries data at TCP level with no DSS sub-option. This > is generally the case when mpext is NULL or mpext->use_map is 0: use a > dedicated value of 'mapping_status' and use it before closing the socket > in subflow_check_data_avail(). Thank you! By chance, do you already have a packetdrill test verifying this? (I guess one similar to the one I recently added, but with a new subflow?) > Link: https://github.com/multipath-tcp/mptcp_net-next/issues/518#issuecomment-2389156143 (I think what's after # is not needed: if that's OK, I can strip that when applying the patch) > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/mptcp/subflow.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 61482f8b425b..77da7a33598a 100644 > --- 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) > @@ -1118,7 +1119,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk, > } > > if (!subflow->map_valid) > - return MAPPING_INVALID; > + return MAPPING_NODSS; This code looks strange when you read it: if ("Invalid") return "No DSS"; Just to be sure: do we need this new "No DSS" case, because "Invalid" is used in other conditions? In other words, do all these MAPPING_INVALID not all point to "middlebox interference" errors? Or in subflow_check_data_avail(), can we not look at subflow->map_valid? subflow->reset_reason = subflow->map_valid ? MPTCP_RST_EMPTCP : MPTCP_RST_EMIDDLEBOX; (It's just to be sure it is really needed to introduce a new status) Cheers, Matt
Hi Davide, On 08/10/2024 19:31, Davide Caratti wrote: > RFC8684 suggests use of "Middlebox interference (code 0x06)" in case of > MPJ subflow that carries data at TCP level with no DSS sub-option. This > is generally the case when mpext is NULL or mpext->use_map is 0: use a > dedicated value of 'mapping_status' and use it before closing the socket > in subflow_check_data_avail(). Thank you for the patch. As discussed on IRC, I just applied your patch with the following changes: - updated the commit message: shorter link + replaced 'MPJ' by 'fully established' - added a comment above 'return MAPPING_NODSS;' - used a ternary operator instead of a switch/case New patches for t/upstream: - 59b3ca4d902c: mptcp: use "middlebox interference" for MP_TCPRST in case map_valid is false - Results: 14705bd9be76..27e5fc4c1ee1 (export) Tests are now in progress: - export: https://github.com/multipath-tcp/mptcp_net-next/commit/694ebec6fa630415fa7ef65a807ebee30232b6b0/checks Cheers, Matt
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 61482f8b425b..77da7a33598a 100644 --- 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) @@ -1118,7 +1119,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk, } if (!subflow->map_valid) - return MAPPING_INVALID; + return MAPPING_NODSS; goto validate_seq; } @@ -1332,7 +1333,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 +1386,13 @@ 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; + switch (status) { + case MAPPING_NODSS: + subflow->reset_reason = MPTCP_RST_EMIDDLEBOX; + break; + default: + subflow->reset_reason = MPTCP_RST_EMPTCP; + } reset: WRITE_ONCE(ssk->sk_err, EBADMSG);
RFC8684 suggests use of "Middlebox interference (code 0x06)" in case of MPJ subflow that carries data at TCP level with no DSS sub-option. This is generally the case when mpext is NULL or mpext->use_map is 0: use a dedicated value of 'mapping_status' and use it before closing the socket in subflow_check_data_avail(). Link: https://github.com/multipath-tcp/mptcp_net-next/issues/518#issuecomment-2389156143 Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/subflow.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)