Re: [PATCH mptcp-next] net/mptcp: don't overwrite sock_ops in mptcp_is_tcpsk()

[Mat Martineau <martineau@kernel.org>]

On Tue, 31 Oct 2023, Davide Caratti wrote:

> Eric suggests:
>
> > The fact that mptcp_is_tcpsk() was able to write over sock->ops was a
> > bit strange to me.
> > mptcp_is_tcpsk() should answer a question, with a read-only argument.
>
> re-factor code to avoid overwriting sock_ops inside that function. Also,
> change the helper name to reflect the semantic, and to disambiguate from
> its dual sk_is_mptcp().
>
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/432
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/protocol.c | 38 ++++++++++++++++++--------------------
> 1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0ad507ac6bc7..9a9f8acd979e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -55,28 +55,15 @@ u64 mptcp_wnd_end(const struct mptcp_sock *msk)
> 	return READ_ONCE(msk->wnd_end);
> }
>
> -static bool mptcp_is_tcpsk(struct sock *sk)
> +static const struct proto_ops *mptcp_get_fallback_tcp_ops(const struct sock *sk)
> {
> -	struct socket *sock = sk->sk_socket;
> -
> -	if (unlikely(sk->sk_prot == &tcp_prot)) {
> -		/* we are being invoked after mptcp_accept() has
> -		 * accepted a non-mp-capable flow: sk is a tcp_sk,
> -		 * not an mptcp one.
> -		 *
> -		 * Hand the socket over to tcp so all further socket ops
> -		 * bypass mptcp.
> -		 */
> -		WRITE_ONCE(sock->ops, &inet_stream_ops);
> -		return true;
> +	if (unlikely(sk->sk_prot == &tcp_prot))
> +		return &inet_stream_ops;
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> -	} else if (unlikely(sk->sk_prot == &tcpv6_prot)) {
> -		WRITE_ONCE(sock->ops, &inet6_stream_ops);
> -		return true;
> +	else if (unlikely(sk->sk_prot == &tcpv6_prot))
> +		return &inet6_stream_ops;
> #endif
> -	}
> -
> -	return false;
> +	return NULL;
> }
>
> static int __mptcp_socket_create(struct mptcp_sock *msk)
> @@ -3832,6 +3819,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> 			       int flags, bool kern)
> {
> 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> +	const struct proto_ops *fallback_ops;
> 	struct sock *ssk, *newsk;
> 	int err;
>
> @@ -3851,7 +3839,8 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> 	lock_sock(newsk);
>
> 	__inet_accept(sock, newsock, newsk);
> -	if (!mptcp_is_tcpsk(newsock->sk)) {
> +	fallback_ops = mptcp_get_fallback_tcp_ops(newsock->sk);
> +	if (!fallback_ops) {
> 		struct mptcp_sock *msk = mptcp_sk(newsk);
> 		struct mptcp_subflow_context *subflow;
>
> @@ -3877,6 +3866,15 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
> 			if (unlikely(list_is_singular(&msk->conn_list)))
> 				inet_sk_state_store(newsk, TCP_CLOSE);
> 		}
> +	} else {
> +		/* we are being invoked after mptcp_accept() has
> +		 * accepted a non-mp-capable flow: sk is a tcp_sk,
> +		 * not an mptcp one.
> +		 *
> +		 * Hand the socket over to tcp so all further socket ops
> +		 * bypass mptcp.
> +		 */
> +		WRITE_ONCE(newsock->sk->sk_socket->ops, fallback_ops);

Hi Davide -

From your comment in the meeting about using a different helper to 
determine if it's a MPTCP or TCP sk, I think the idea is to get rid of the 
helper function (rather than rename) and then include the ipv4/ipv6 logic 
in this block of code? That approach sounds good to me.

If that doesn't work out, another option is to rename the helper function 
to something that meets Eric's criteria (avoiding "mptcp_is_xxx()" or 
"mptcp_get_xxx()" names).

- Mat

> 	}
> 	release_sock(newsk);
>
> -- 
> 2.41.0
>
>
>