Re: [PATCH mptcp-net] mptcp: fix locking in mptcp_nl_cmd_sf_destroy()

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Mon, 27 Jun 2022, Paolo Abeni wrote:

> The user-space PM subflow removal path uses a couple of helpers
> that must be called under the msk socket lock and the current
> code lacks such requirement.
>
> Change the existing lock scope so that the relevant code is under
> its protection.
>
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> It should close issues/287, let's see what the CI says

Thanks for tracking this down, Paolo. Patch looks good, tests are passing 
for me.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> ---
> net/mptcp/pm_userspace.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 3d1d365e9c6f..33be79f0e9c2 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -307,15 +307,11 @@ static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk,
> 				      const struct mptcp_addr_info *local,
> 				      const struct mptcp_addr_info *remote)
> {
> -	struct sock *sk = &msk->sk.icsk_inet.sk;
> 	struct mptcp_subflow_context *subflow;
> -	struct sock *found = NULL;
>
> 	if (local->family != remote->family)
> 		return NULL;
>
> -	lock_sock(sk);
> -
> 	mptcp_for_each_subflow(msk, subflow) {
> 		const struct inet_sock *issk;
> 		struct sock *ssk;
> @@ -348,16 +344,11 @@ static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk,
> 		}
>
> 		if (issk->inet_sport == local->port &&
> -		    issk->inet_dport == remote->port) {
> -			found = ssk;
> -			goto found;
> -		}
> +		    issk->inet_dport == remote->port)
> +			return ssk;
> 	}
>
> -found:
> -	release_sock(sk);
> -
> -	return found;
> +	return NULL;
> }
>
> int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
> @@ -413,6 +404,7 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
> 	}
>
> 	sk = &msk->sk.icsk_inet.sk;
> +	lock_sock(sk);
> 	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
> 	if (ssk) {
> 		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> @@ -424,8 +416,9 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
> 	} else {
> 		err = -ESRCH;
> 	}
> +	release_sock(sk);
>
> - destroy_err:
> +destroy_err:
> 	sock_put((struct sock *)msk);
> 	return err;
> }
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel