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(-)

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);
-- 
2.46.0

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
-- 
Sponsored by the NGI0 Core fund.

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
-- 
Sponsored by the NGI0 Core fund.