Christoph reports a lockdep splat in the mptcp_subflow_create_socket()
error path, when such function is invoked by
mptcp_pm_nl_create_listen_socket().

Such code path acquires two separates, nested socket lock, with the
internal lock operation lacking the "nested" annotation. Adding that
in sock_release() for mptcp's sake only could be confusing.

Instead we can revert the commit introducing the outermost lock as the
relevant socket is not exposted in any way to ther user not to the
stack and can't be touched by any other entity.

Reported-by: Christoph Paasch <cpaasch@apple.com>
Fixes: ad2171009d96 ("mptcp: fix locking for in-kernel listener creation")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/354
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 14 ++++++++------
 net/mptcp/subflow.c    |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d1d859517d91..54b0aeb8bb01 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -999,8 +999,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 {
 	int addrlen = sizeof(struct sockaddr_in);
 	struct sockaddr_storage addr;
+	struct mptcp_sock *msk;
 	struct socket *ssock;
-	struct sock *newsk;
 	int backlog = 1024;
 	int err;
 
@@ -1009,13 +1009,15 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	if (err)
 		return err;
 
-	newsk = entry->lsk->sk;
-	if (!newsk)
+	msk = mptcp_sk(entry->lsk->sk);
+	if (!msk)
 		return -EINVAL;
 
-	lock_sock(newsk);
-	ssock = __mptcp_nmpc_socket(mptcp_sk(newsk));
-	release_sock(newsk);
+	/* No other code-paths can touch the msk at this point and acquiring
+	 * the msk the socket lock will trigger a lockdep false positive in
+	 * the mptcp_subflow_create_socket error path.
+	 */
+	ssock = __mptcp_nmpc_socket(msk);
 	if (IS_ERR(ssock))
 		return PTR_ERR(ssock);
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 28c64811a8af..90895733860f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1708,7 +1708,7 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 	if (err)
 		return err;
 
-	lock_sock_nested(sf->sk, SINGLE_DEPTH_NESTING);
+	lock_sock(sf->sk);
 
 	err = security_mptcp_add_subflow(sk, sf->sk);
 	if (err)
-- 
2.39.2

On Mon, 2023-02-20 at 17:10 +0100, Paolo Abeni wrote:
> Christoph reports a lockdep splat in the mptcp_subflow_create_socket()
> error path, when such function is invoked by
> mptcp_pm_nl_create_listen_socket().
> 
> Such code path acquires two separates, nested socket lock, with the
> internal lock operation lacking the "nested" annotation. Adding that
> in sock_release() for mptcp's sake only could be confusing.
> 
> Instead we can revert the commit introducing the outermost lock as the
> relevant socket is not exposted in any way to ther user not to the
> stack and can't be touched by any other entity.
> 
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Fixes: ad2171009d96 ("mptcp: fix locking for in-kernel listener creation")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/354
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/pm_netlink.c | 14 ++++++++------
>  net/mptcp/subflow.c    |  2 +-
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d1d859517d91..54b0aeb8bb01 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -999,8 +999,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  {
>  	int addrlen = sizeof(struct sockaddr_in);
>  	struct sockaddr_storage addr;
> +	struct mptcp_sock *msk;
>  	struct socket *ssock;
> -	struct sock *newsk;
>  	int backlog = 1024;
>  	int err;
>  
> @@ -1009,13 +1009,15 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  	if (err)
>  		return err;
>  
> -	newsk = entry->lsk->sk;
> -	if (!newsk)
> +	msk = mptcp_sk(entry->lsk->sk);
> +	if (!msk)
>  		return -EINVAL;
>  
> -	lock_sock(newsk);
> -	ssock = __mptcp_nmpc_socket(mptcp_sk(newsk));

This can end-up calling mptcp_sockopt_sync() which in turn explicitly
check for the msk socket ownership. So something different is required
:(

I still would try to avoid adding the nested annotation in
sock_release()

/P