Re: [PATCH mptcp-next v2 2/2] mptcp: setsockopt support for TCP_MD5SIG

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2025-07-31 at 11:59 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the new version.
> 
> On 31/07/2025 09:27, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This adds setsockopt support for TCP_MD5SIG and TCP_MD5SIG_EXT
> > options.
> > The implementation:
> > - Allows setting these options (getsockopt remains unsupported)
> > - Applies them only to the first subflow
> > - Forces fallback to TCP (since MD5 isn't compatible with MPTCP)
> > 
> > Setting these options triggers fallback to TCP to maintain MD5
> > compatibility.
> > 
> > Note that TCP_MD5SIG and TCP_MD5SIG_EXT are unsupported for TCP
> > too.
> > 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/575
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/sockopt.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> > index b264185b810d..3ffdeca694be 100644
> > --- a/net/mptcp/sockopt.c
> > +++ b/net/mptcp/sockopt.c
> > @@ -13,6 +13,7 @@
> >  #include <net/tcp.h>
> >  #include <net/mptcp.h>
> >  #include "protocol.h"
> > +#include "mib.h"
> >  
> >  #define MIN_INFO_OPTLEN_SIZE		16
> >  #define MIN_FULL_INFO_OPTLEN_SIZE	40
> > @@ -567,11 +568,12 @@ static bool mptcp_supported_sockopt(int
> > level, int optname)
> >  		case TCP_FASTOPEN_CONNECT:
> >  		case TCP_FASTOPEN_KEY:
> >  		case TCP_FASTOPEN_NO_COOKIE:
> > +		/* MD5 will force a fallback to TCP: OK to set
> > while not connected */
> > +		case TCP_MD5SIG:
> > +		case TCP_MD5SIG_EXT:
> >  			return true;
> >  		}
> >  
> > -		/* TCP_MD5SIG, TCP_MD5SIG_EXT are not supported,
> > MD5 is not compatible with MPTCP */
> > -
> >  		/* TCP_REPAIR, TCP_REPAIR_QUEUE, TCP_QUEUE_SEQ,
> > TCP_REPAIR_OPTIONS,
> >  		 * TCP_REPAIR_WINDOW are not supported, better
> > avoid this mess
> >  		 */
> > @@ -830,6 +832,12 @@ static int mptcp_setsockopt_sol_tcp(struct
> > mptcp_sock *msk, int optname,
> >  		/* See tcp.c: TCP_DEFER_ACCEPT does not fail */
> >  		mptcp_setsockopt_first_sf_only(msk, SOL_TCP,
> > optname, optval, optlen);
> >  		return 0;
> > +#ifdef CONFIG_TCP_MD5SIG
> > +	case TCP_MD5SIG:
> > +	case TCP_MD5SIG_EXT:
> > +		__mptcp_try_fallback(msk,
> > MPTCP_MIB_MD5SIGFALLBACK);
> 
> I don't think that's a good idea to do just that: here, you will do a
> fallback even if the options are not correct. Please this helper can
> return false if a fallback is not possible, and a reset will be
> needed.

Yes, here the return value of __mptcp_try_fallback() needs to be
checked:

 if (!__mptcp_try_fallback(msk, MPTCP_MIB_MD5SIGFALLBACK))
	 return 0;

> 
> I think it would be easier to simply limit the use of this option for
> listened and closed state: 

Yes, the state of sk needs to be checked too:

 if (!((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
     !__mptcp_try_fallback(msk, MPTCP_MIB_MD5SIGFALLBACK))
 	return 0;

Do you think this will work?

> a fallback will be done in mptcp_connect()
> and subflow_check_req().

When running the selftests of patch 3 in v1, no fallback is done in
mptcp_connect() and subflow_check_req(). Since both
rcu_access_pointer(tcp_sk(ssk)->md5sig_info) and
rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info) are NULL. So we do
need to fallback in mptcp_setsockopt_sol_tcp().

Thanks,
-Geliang

> 
> If I'm not mistaken, mptcp_setsockopt_first_sf_only() will limit to
> the
> first subflow, before the establishment of the connection
> (__mptcp_nmpc_sk() is explicitly checking the state), no?
> 
> Then all you require is to add the two 'case', no? If yes, please add
> a
> Fixes tag:
> 
> Fixes: d9e4c1291810 ("mptcp: only admit explicitly supported
> sockopt")
> 
> And add something like this in the commit message:
> 
>   Supporting TCP_MD5 socket option is required when MPTCP is used by
>   default when creating a socket, to keep the same behaviour as with
>   TCP. TCP_MD5 is not compatible with MPTCP, and it will cause a
>   fallback to TCP at the connection request, if MPTCP was requested.
>   This then fixes a "regression" compared to TCP.
> 
> 
> One last thing, please also send the new packetdrill test on GitHub
> when
> sending a next version.
> 
> Cheers,
> Matt