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 just add a new lockclass to the in-kernel msk socket,
re-initializing the lockdep infra after the socket creation.

Reported-by: Christoph Paasch <cpaasch@apple.com>
Fixes: ad2171009d96 ("mptcp: fix locking for in-kernel listener creation")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d1d859517d91..6048fdee404b 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -994,9 +994,13 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 	return ret;
 }
 
+static struct lock_class_key mptcp_slock_keys[2];
+static struct lock_class_key mptcp_keys[2];
+
 static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 					    struct mptcp_pm_addr_entry *entry)
 {
+	bool is_ipv6 = sk->sk_family == AF_INET6;
 	int addrlen = sizeof(struct sockaddr_in);
 	struct sockaddr_storage addr;
 	struct socket *ssock;
@@ -1013,6 +1017,19 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	if (!newsk)
 		return -EINVAL;
 
+	/* The subflow socket lock is acquired in a nested to the msk one
+	 * in several places, even by the TCP stack, and this msk is a kernel
+	 * socket: lockdep complains. Instead of propagating the _nested
+	 * modifiers in several places, re-init the lock class for the msk
+	 * socket to an mptcp specific one.
+	 */
+	sock_lock_init_class_and_name(
+			newsk,
+			is_ipv6 ? "mlock-AF_INET6" : "mlock-AF_INET",
+			&mptcp_slock_keys[is_ipv6],
+			is_ipv6 ? "msk_lock-AF_INET6": "msk_lock-AF_INET",
+			&mptcp_keys[is_ipv6]);
+
 	lock_sock(newsk);
 	ssock = __mptcp_nmpc_socket(mptcp_sk(newsk));
 	release_sock(newsk);
-- 
2.39.2

On Tue, 2023-02-28 at 17:19 +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 just add a new lockclass to the in-kernel msk socket,
> re-initializing the lockdep infra after the socket creation.
> 
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Fixes: ad2171009d96 ("mptcp: fix locking for in-kernel listener creation")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Sorry for the unintentional dup. PEBKAC hit again here :(

I'll drop this one from PW.

/P

Hi Paolo, Christoph,

On 24/02/2023 13:01, 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 just add a new lockclass to the in-kernel msk socket,
> re-initializing the lockdep infra after the socket creation.
> 
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Fixes: ad2171009d96 ("mptcp: fix locking for in-kernel listener creation")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thank you for the patch and for having tested it!

I just applied this patch in our tree (fixes for -net) with Christoph's
Tested-by tag and without the issues reported by checkpatch.pl (I hope
that's OK).

New patches for t/upstream-net and t/upstream:
- 6f002c31c364: mptcp: fix lockdep false positive in
mptcp_pm_nl_create_listen_socket()
- Results: 48ec0932754d..8d03cb1e10a4 (export-net)
- Results: 3bf7fef56098..f84e6823c4bb (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230227T211242
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230227T211242

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Hi Paolo,

On 24/02/2023 13:01, 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 just add a new lockclass to the in-kernel msk socket,
> re-initializing the lockdep infra after the socket creation.
> 
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Fixes: ad2171009d96 ("mptcp: fix locking for in-kernel listener creation")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thank you for the fix!

It looks "simpler" than expected, no? :)

The modification seems to make sense to me:

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

I guess we can also add:

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/354

It would be good if Christoph could validate this modification :)
I can wait a bit before including this patch.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Fri, 2023-02-24 at 15:02 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 24/02/2023 13:01, 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 just add a new lockclass to the in-kernel msk socket,
> > re-initializing the lockdep infra after the socket creation.
> > 
> > Reported-by: Christoph Paasch <cpaasch@apple.com>
> > Fixes: ad2171009d96 ("mptcp: fix locking for in-kernel listener creation")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Thank you for the fix!
> 
> It looks "simpler" than expected, no? :)

Yep, but I did a few other more complex attempts first.

I think this one should stage a bit in the export branch, to ensure
there are no side effects.

Thanks!

Paolo