Re: [PATCH mptcp-next] mptcp: use "middlebox interference" for MP_TCPRST in case map_valid is false

[Matthieu Baerts <matttbe@kernel.org>]

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.