* [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys
2024-09-05 17:03 [syzbot] [mptcp?] possible deadlock in sk_clone_lock (3) syzbot
@ 2024-09-11 8:35 ` Matthieu Baerts (NGI0)
2024-09-11 9:06 ` Matthieu Baerts (NGI0)
2024-09-11 9:06 ` [syzbot] [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys syzbot
2 siblings, 0 replies; 5+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-09-11 8:35 UTC (permalink / raw)
To: syzbot+f4aacdfef2c6a6529c3e; +Cc: patches
From: Cong Wang <cong.wang@bytedance.com>
In mptcp_pm_nl_create_listen_socket(), we already initialize mptcp sock
lock with mptcp_slock_keys and mptcp_keys. But that is not sufficient,
at least mptcp_init_sock() and mptcp_sk_clone_init() still miss it.
As reported by syzbot, mptcp_sk_clone_init() is challenging due to that
sk_clone_lock() immediately locks the new sock after preliminary
initialization. To amend that, introduce ->init_clone() for struct proto
and call it right after the sock_lock_init(), so now mptcp sock could
initialize the sock lock again with its own lockdep keys.
This patch does not fix any real deadlock, it only makes lockdep happy.
Fixes: 58b09919626b ("mptcp: create msk early")
Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f4aacdfef2c6a6529c3e
Cc: Matthieu Baerts <matttbe@kernel.org>
Cc: Mat Martineau <martineau@kernel.org>
Cc: Geliang Tang <geliang@kernel.org>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
Link: https://lore.kernel.org/r/20240911042425.978665-1-xiyou.wangcong@gmail.com
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
include/net/sock.h | 1 +
net/core/sock.c | 2 ++
net/mptcp/pm_netlink.c | 16 +---------------
net/mptcp/protocol.c | 22 ++++++++++++++++++++++
net/mptcp/protocol.h | 1 +
5 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index f51d61fab059..2e06e720071a 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1226,6 +1226,7 @@ struct proto {
int (*ioctl)(struct sock *sk, int cmd,
int *karg);
int (*init)(struct sock *sk);
+ void (*init_clone)(struct sock *sk);
void (*destroy)(struct sock *sk);
void (*shutdown)(struct sock *sk, int how);
int (*setsockopt)(struct sock *sk, int level,
diff --git a/net/core/sock.c b/net/core/sock.c
index 468b1239606c..d37e6efc7f91 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2325,6 +2325,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
}
sk_node_init(&newsk->sk_node);
sock_lock_init(newsk);
+ if (prot->init_clone)
+ prot->init_clone(newsk);
bh_lock_sock(newsk);
newsk->sk_backlog.head = newsk->sk_backlog.tail = NULL;
newsk->sk_backlog.len = 0;
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index fe34297ea6dc..a82b68cc1faf 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1067,13 +1067,9 @@ 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 sock *newsk, *ssk;
@@ -1089,17 +1085,7 @@ 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]);
+ mptcp_sock_lock_init(newsk);
lock_sock(newsk);
ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3b837765c84b..7ecaa83794b2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2839,6 +2839,7 @@ static int mptcp_init_sock(struct sock *sk)
int ret;
__mptcp_init_sock(sk);
+ mptcp_sock_lock_init(sk);
if (!mptcp_is_enabled(net))
return -ENOPROTOOPT;
@@ -2865,6 +2866,26 @@ static int mptcp_init_sock(struct sock *sk)
return 0;
}
+static struct lock_class_key mptcp_slock_keys[2];
+static struct lock_class_key mptcp_keys[2];
+
+void mptcp_sock_lock_init(struct sock *sk)
+{
+ bool is_ipv6 = sk->sk_family == AF_INET6;
+
+ /* 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(sk,
+ 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]);
+}
+
static void __mptcp_clear_xmit(struct sock *sk)
{
struct mptcp_sock *msk = mptcp_sk(sk);
@@ -3797,6 +3818,7 @@ static struct proto mptcp_prot = {
.name = "MPTCP",
.owner = THIS_MODULE,
.init = mptcp_init_sock,
+ .init_clone = mptcp_sock_lock_init,
.connect = mptcp_connect,
.disconnect = mptcp_disconnect,
.close = mptcp_close,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index c3942416fa3a..819a193c0d88 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -812,6 +812,7 @@ void __init mptcp_proto_init(void);
int __init mptcp_proto_v6_init(void);
#endif
+void mptcp_sock_lock_init(struct sock *sk);
struct sock *mptcp_sk_clone_init(const struct sock *sk,
const struct mptcp_options_received *mp_opt,
struct sock *ssk,
--
2.45.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys
2024-09-05 17:03 [syzbot] [mptcp?] possible deadlock in sk_clone_lock (3) syzbot
2024-09-11 8:35 ` [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys Matthieu Baerts (NGI0)
@ 2024-09-11 9:06 ` Matthieu Baerts (NGI0)
2024-09-11 9:27 ` [syzbot] [mptcp?] possible deadlock in sk_clone_lock (3) syzbot
2024-09-11 9:06 ` [syzbot] [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys syzbot
2 siblings, 1 reply; 5+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-09-11 9:06 UTC (permalink / raw)
To: syzbot+f4aacdfef2c6a6529c3e; +Cc: patches
From: Cong Wang <cong.wang@bytedance.com>
In mptcp_pm_nl_create_listen_socket(), we already initialize mptcp sock
lock with mptcp_slock_keys and mptcp_keys. But that is not sufficient,
at least mptcp_init_sock() and mptcp_sk_clone_init() still miss it.
As reported by syzbot, mptcp_sk_clone_init() is challenging due to that
sk_clone_lock() immediately locks the new sock after preliminary
initialization. To amend that, introduce ->init_clone() for struct proto
and call it right after the sock_lock_init(), so now mptcp sock could
initialize the sock lock again with its own lockdep keys.
This patch does not fix any real deadlock, it only makes lockdep happy.
#syz test
Fixes: 58b09919626b ("mptcp: create msk early")
Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug\?extid\=f4aacdfef2c6a6529c3e
Cc: Matthieu Baerts <matttbe@kernel.org>
Cc: Mat Martineau <martineau@kernel.org>
Cc: Geliang Tang <geliang@kernel.org>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
Link: https://lore.kernel.org/r/20240911042425.978665-1-xiyou.wangcong@gmail.com
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
include/net/sock.h | 1 +
net/core/sock.c | 2 ++
net/mptcp/pm_netlink.c | 16 +---------------
net/mptcp/protocol.c | 22 ++++++++++++++++++++++
net/mptcp/protocol.h | 1 +
5 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index f51d61fab059..2e06e720071a 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1226,6 +1226,7 @@ struct proto {
int (*ioctl)(struct sock *sk, int cmd,
int *karg);
int (*init)(struct sock *sk);
+ void (*init_clone)(struct sock *sk);
void (*destroy)(struct sock *sk);
void (*shutdown)(struct sock *sk, int how);
int (*setsockopt)(struct sock *sk, int level,
diff --git a/net/core/sock.c b/net/core/sock.c
index 468b1239606c..d37e6efc7f91 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2325,6 +2325,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
}
sk_node_init(&newsk->sk_node);
sock_lock_init(newsk);
+ if (prot->init_clone)
+ prot->init_clone(newsk);
bh_lock_sock(newsk);
newsk->sk_backlog.head = newsk->sk_backlog.tail = NULL;
newsk->sk_backlog.len = 0;
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index fe34297ea6dc..a82b68cc1faf 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1067,13 +1067,9 @@ 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 sock *newsk, *ssk;
@@ -1089,17 +1085,7 @@ 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]);
+ mptcp_sock_lock_init(newsk);
lock_sock(newsk);
ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3b837765c84b..7ecaa83794b2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2839,6 +2839,7 @@ static int mptcp_init_sock(struct sock *sk)
int ret;
__mptcp_init_sock(sk);
+ mptcp_sock_lock_init(sk);
if (!mptcp_is_enabled(net))
return -ENOPROTOOPT;
@@ -2865,6 +2866,26 @@ static int mptcp_init_sock(struct sock *sk)
return 0;
}
+static struct lock_class_key mptcp_slock_keys[2];
+static struct lock_class_key mptcp_keys[2];
+
+void mptcp_sock_lock_init(struct sock *sk)
+{
+ bool is_ipv6 = sk->sk_family == AF_INET6;
+
+ /* 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(sk,
+ 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]);
+}
+
static void __mptcp_clear_xmit(struct sock *sk)
{
struct mptcp_sock *msk = mptcp_sk(sk);
@@ -3797,6 +3818,7 @@ static struct proto mptcp_prot = {
.name = "MPTCP",
.owner = THIS_MODULE,
.init = mptcp_init_sock,
+ .init_clone = mptcp_sock_lock_init,
.connect = mptcp_connect,
.disconnect = mptcp_disconnect,
.close = mptcp_close,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index c3942416fa3a..819a193c0d88 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -812,6 +812,7 @@ void __init mptcp_proto_init(void);
int __init mptcp_proto_v6_init(void);
#endif
+void mptcp_sock_lock_init(struct sock *sk);
struct sock *mptcp_sk_clone_init(const struct sock *sk,
const struct mptcp_options_received *mp_opt,
struct sock *ssk,
--
2.45.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [syzbot] [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys
2024-09-05 17:03 [syzbot] [mptcp?] possible deadlock in sk_clone_lock (3) syzbot
2024-09-11 8:35 ` [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys Matthieu Baerts (NGI0)
2024-09-11 9:06 ` Matthieu Baerts (NGI0)
@ 2024-09-11 9:06 ` syzbot
2 siblings, 0 replies; 5+ messages in thread
From: syzbot @ 2024-09-11 9:06 UTC (permalink / raw)
To: linux-kernel, syzkaller-bugs
For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.
***
Subject: [PATCH net v2] mptcp: initialize sock lock with its own lockdep keys
Author: matttbe@kernel.org
From: Cong Wang <cong.wang@bytedance.com>
In mptcp_pm_nl_create_listen_socket(), we already initialize mptcp sock
lock with mptcp_slock_keys and mptcp_keys. But that is not sufficient,
at least mptcp_init_sock() and mptcp_sk_clone_init() still miss it.
As reported by syzbot, mptcp_sk_clone_init() is challenging due to that
sk_clone_lock() immediately locks the new sock after preliminary
initialization. To amend that, introduce ->init_clone() for struct proto
and call it right after the sock_lock_init(), so now mptcp sock could
initialize the sock lock again with its own lockdep keys.
This patch does not fix any real deadlock, it only makes lockdep happy.
#syz test
Fixes: 58b09919626b ("mptcp: create msk early")
Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug\?extid\=f4aacdfef2c6a6529c3e
Cc: Matthieu Baerts <matttbe@kernel.org>
Cc: Mat Martineau <martineau@kernel.org>
Cc: Geliang Tang <geliang@kernel.org>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
Link: https://lore.kernel.org/r/20240911042425.978665-1-xiyou.wangcong@gmail.com
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
include/net/sock.h | 1 +
net/core/sock.c | 2 ++
net/mptcp/pm_netlink.c | 16 +---------------
net/mptcp/protocol.c | 22 ++++++++++++++++++++++
net/mptcp/protocol.h | 1 +
5 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index f51d61fab059..2e06e720071a 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1226,6 +1226,7 @@ struct proto {
int (*ioctl)(struct sock *sk, int cmd,
int *karg);
int (*init)(struct sock *sk);
+ void (*init_clone)(struct sock *sk);
void (*destroy)(struct sock *sk);
void (*shutdown)(struct sock *sk, int how);
int (*setsockopt)(struct sock *sk, int level,
diff --git a/net/core/sock.c b/net/core/sock.c
index 468b1239606c..d37e6efc7f91 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2325,6 +2325,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
}
sk_node_init(&newsk->sk_node);
sock_lock_init(newsk);
+ if (prot->init_clone)
+ prot->init_clone(newsk);
bh_lock_sock(newsk);
newsk->sk_backlog.head = newsk->sk_backlog.tail = NULL;
newsk->sk_backlog.len = 0;
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index fe34297ea6dc..a82b68cc1faf 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1067,13 +1067,9 @@ 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 sock *newsk, *ssk;
@@ -1089,17 +1085,7 @@ 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]);
+ mptcp_sock_lock_init(newsk);
lock_sock(newsk);
ssk = __mptcp_nmpc_sk(mptcp_sk(newsk));
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3b837765c84b..7ecaa83794b2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2839,6 +2839,7 @@ static int mptcp_init_sock(struct sock *sk)
int ret;
__mptcp_init_sock(sk);
+ mptcp_sock_lock_init(sk);
if (!mptcp_is_enabled(net))
return -ENOPROTOOPT;
@@ -2865,6 +2866,26 @@ static int mptcp_init_sock(struct sock *sk)
return 0;
}
+static struct lock_class_key mptcp_slock_keys[2];
+static struct lock_class_key mptcp_keys[2];
+
+void mptcp_sock_lock_init(struct sock *sk)
+{
+ bool is_ipv6 = sk->sk_family == AF_INET6;
+
+ /* 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(sk,
+ 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]);
+}
+
static void __mptcp_clear_xmit(struct sock *sk)
{
struct mptcp_sock *msk = mptcp_sk(sk);
@@ -3797,6 +3818,7 @@ static struct proto mptcp_prot = {
.name = "MPTCP",
.owner = THIS_MODULE,
.init = mptcp_init_sock,
+ .init_clone = mptcp_sock_lock_init,
.connect = mptcp_connect,
.disconnect = mptcp_disconnect,
.close = mptcp_close,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index c3942416fa3a..819a193c0d88 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -812,6 +812,7 @@ void __init mptcp_proto_init(void);
int __init mptcp_proto_v6_init(void);
#endif
+void mptcp_sock_lock_init(struct sock *sk);
struct sock *mptcp_sk_clone_init(const struct sock *sk,
const struct mptcp_options_received *mp_opt,
struct sock *ssk,
--
2.45.2
^ permalink raw reply related [flat|nested] 5+ messages in thread