linux-kernel - Re: [PATCH net 2/3] mptcp: reset when MPTCP opts are dropped after join

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD501Os6sdNrc0PaD8Lnew1UO8nWRVd+0z=DgLEuUttus3aDQw@mail.gmail.com>
Date: Tue, 25 Feb 2025 12:04:41 +0000
From: "Chester A. Unal" <chester.a.unal@...dite-tech.com>
To: "Matthieu Baerts (NGI0)" <matttbe@...nel.org>
Cc: mptcp@...ts.linux.dev, Mat Martineau <martineau@...nel.org>, 
	Geliang Tang <geliang@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org, Zenon van Deventer <zenon@...dite-tech.com>, 
	dwayne Du Preez <dwayne.dupreez@...dite-tech.com>, 
	Alexander Scholten <alexander.scholten@...dite-tech.com>
Subject: Re: [PATCH net 2/3] mptcp: reset when MPTCP opts are dropped after join

Tested-by: Chester A. Unal <chester.a.unal@...dite-tech.com>

Thanks a ton!
Chester A.

On Mon, Feb 24, 2025 at 6:19 PM Matthieu Baerts (NGI0)
<matttbe@...nel.org> wrote:
>
> Before this patch, if the checksum was not used, the subflow was only
> reset if map_data_len was != 0. If there were no MPTCP options or an
> invalid mapping, map_data_len was not set to the data len, and then the
> subflow was not reset as it should have been, leaving the MPTCP
> connection in a wrong fallback mode.
>
> This map_data_len condition has been introduced to handle the reception
> of the infinite mapping. Instead, a new dedicated mapping error could
> have been returned and treated as a special case. However, the commit
> 31bf11de146c ("mptcp: introduce MAPPING_BAD_CSUM") has been introduced
> by Paolo Abeni soon after, and backported later on to stable. It better
> handle the csum case, and it means the exception for valid_csum_seen in
> subflow_can_fallback(), plus this one for the infinite mapping in
> subflow_check_data_avail(), are no longer needed.
>
> In other words, the code can be simplified there: a fallback should only
> be done if msk->allow_infinite_fallback is set. This boolean is set to
> false once MPTCP-specific operations acting on the whole MPTCP
> connection vs the initial path have been done, e.g. a second path has
> been created, or an MPTCP re-injection -- yes, possible even with a
> single subflow. The subflow_can_fallback() helper can then be dropped,
> and replaced by this single condition.
>
> This also makes the code clearer: a fallback should only be done if it
> is possible to do so.
>
> While at it, no need to set map_data_len to 0 in get_mapping_status()
> for the infinite mapping case: it will be set to skb->len just after, at
> the end of subflow_check_data_avail(), and not read in between.
>
> Fixes: f8d4bcacff3b ("mptcp: infinite mapping receiving")
> Cc: stable@...r.kernel.org
> Reported-by: Chester A. Unal <chester.a.unal@...dite-tech.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/544
> Acked-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>
> ---
>  net/mptcp/subflow.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index dfcbef9c46246983d21c827d9551d4eb2c95430e..9f18217dddc865bc467a2c5c34aa4bf23bf3ac75 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1142,7 +1142,6 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
>         if (data_len == 0) {
>                 pr_debug("infinite mapping received\n");
>                 MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
> -               subflow->map_data_len = 0;
>                 return MAPPING_INVALID;
>         }
>
> @@ -1286,18 +1285,6 @@ static void subflow_sched_work_if_closed(struct mptcp_sock *msk, struct sock *ss
>                 mptcp_schedule_work(sk);
>  }
>
> -static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
> -{
> -       struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> -
> -       if (subflow->mp_join)
> -               return false;
> -       else if (READ_ONCE(msk->csum_enabled))
> -               return !subflow->valid_csum_seen;
> -       else
> -               return READ_ONCE(msk->allow_infinite_fallback);
> -}
> -
>  static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk)
>  {
>         struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> @@ -1393,7 +1380,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>                         return true;
>                 }
>
> -               if (!subflow_can_fallback(subflow) && subflow->map_data_len) {
> +               if (!READ_ONCE(msk->allow_infinite_fallback)) {
>                         /* fatal protocol error, close the socket.
>                          * subflow_error_report() will introduce the appropriate barriers
>                          */
>
> --
> 2.47.1
>