Re: [PATCH mptcp-next v1 1/2] mptcp: add TCP_MAXSEG sockopt support

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/04/2025 11:30, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The TCP_MAXSEG socket option is currently not supported by MPTCP, mainly
> because it has never been requested before. But there are still valid
> use-cases, e.g. with HAProxy.
> 
> This patch adds its support in MPTCP by propagating the value to all
> subflows.

Thank you for looking at that.

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/515
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/sockopt.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 3caa0a9d3b38..3014713b037c 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -798,6 +798,24 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>  	return ret;
>  }
>  
> +static int mptcp_setsockopt_sol_tcp_maxseg(struct mptcp_sock *msk,
> +					   sockptr_t optval,
> +					   unsigned int optlen)

Might be good to have a generic helper, similar to
mptcp_setsockopt_first_sf_only(), no?

  mptcp_setsockopt_all_subflows()

> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	mptcp_for_each_subflow(msk, subflow) {

If there is no subflows (listening sockets?), you might need to call
mptcp_setsockopt_first_sf_only(), no?

> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		int ret;
> +
> +		ret = tcp_setsockopt(ssk, SOL_TCP, TCP_MAXSEG, optval, optlen);

No need to lock around this call?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  				    sockptr_t optval, unsigned int optlen)
>  {
> @@ -819,6 +837,8 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  	case TCP_FASTOPEN_NO_COOKIE:
>  		return mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname,
>  						      optval, optlen);
> +	case TCP_MAXSEG:
> +		return mptcp_setsockopt_sol_tcp_maxseg(msk, optval, optlen);
>  	}
>  
>  	ret = mptcp_get_int_option(msk, optval, optlen, &val);
> @@ -1368,6 +1388,13 @@ static int mptcp_put_int_option(struct mptcp_sock *msk, char __user *optval,
>  	return 0;
>  }
>  
> +static int mptcp_getsockopt_sol_tcp_maxseg(struct mptcp_sock *msk,
> +					   char __user *optval,
> +					   int __user *optlen)
> +{
> +	return tcp_getsockopt(msk->first, SOL_TCP, TCP_MAXSEG, optval, optlen);

Same here: no need to lock? Also, msk->first can be NULL for listening
sockets, no?

Anyway, I think it is best not to change the way the socket options are
handled today: the socket option is propagated to existing and future
subflows. Look how "msk->keepalive_intvl" is handled in sockopt.c for
example.

- setsockopt: store the value (in case of success) in the msk.
- getsockopt: return this value (mptcp_put_int_option())

WDYT?

> +}
> +
>  static int mptcp_getsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  				    char __user *optval, int __user *optlen)
>  {
> @@ -1407,6 +1434,8 @@ static int mptcp_getsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  		return mptcp_put_int_option(msk, optval, optlen, msk->notsent_lowat);
>  	case TCP_IS_MPTCP:
>  		return mptcp_put_int_option(msk, optval, optlen, 1);
> +	case TCP_MAXSEG:
> +		return mptcp_getsockopt_sol_tcp_maxseg(msk, optval, optlen);
>  	}
>  	return -EOPNOTSUPP;
>  }

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.