| Message ID | 20240911042425.978665-1-xiyou.wangcong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | [net,v2] mptcp: initialize sock lock with its own lockdep keys | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 93 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi Cong,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10805076657
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/cff8cdcc8f33
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=889140
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Cong Wang, On 11/09/2024 06:24, Cong Wang wrote: > 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> > --- > v2: Address Matthieu's comments Thank you for the v2! (...) > diff --git a/net/core/sock.c b/net/core/sock.c > index 9abc4fe25953..747d7e479d69 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); As mentioned on the v1, if the idea is to introduce a new hook here, maybe it can replace the call to sock_lock_init() when this new function pointer is defined. In this case, it could be renamed 'lock_init' or 'init_lock'. But no need to send a new version for the moment, please see below. I suggest the following plan if that OK: because the initial report was a false positive, there are modification of the core, and because we are a bit in the middle of other things, I suggest not rushing up with this patch. We can look at it in more details when the other things will calm down, very likely in ~2 weeks, after LPC. Cheers, Matt
On Wed, Sep 11, 2024 at 1:25 AM Matthieu Baerts <matttbe@kernel.org> wrote: > > diff --git a/net/core/sock.c b/net/core/sock.c > > index 9abc4fe25953..747d7e479d69 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); > As mentioned on the v1, if the idea is to introduce a new hook here, > maybe it can replace the call to sock_lock_init() when this new function > pointer is defined. In this case, it could be renamed 'lock_init' or > 'init_lock'. But no need to send a new version for the moment, please > see below. Two reasons why I don't think it is a better name: 1. It indicates we only initialize the lock, but the parameter we pass to it is struct sock pointer, not any lock pointer. 2. We have to distinguish clone path from regular sock init path, otherwise people may interpret as if it were called for every initialization. Hence, mentioning 'clone' in the name is very necessary. > > I suggest the following plan if that OK: because the initial report was > a false positive, there are modification of the core, and because we are > a bit in the middle of other things, I suggest not rushing up with this > patch. We can look at it in more details when the other things will calm > down, very likely in ~2 weeks, after LPC. Sure, no rush at all. Like I already mentioned, this patch should not target -stable, possibly not even -net. Thanks.
On 9/11/24 06:24, Cong Wang wrote: > 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. Indeed the syzbot reported splat is a false positive. However the splat show a real problem: the locks acquired are in order the signal endpoint tcp listener lock a newly created msk lock, that means that the signal endpoint tcp listener allows successful MPC handshake completion. That in turn is a bad thing, as the subflow sockets created by such handshake will never be accepted, the client will stuck. Additionally, when the listener queue will be full, not even MPJ subflow will be accepted. We need to explicitly make such MPC handshakes fail. That in turn will also avoid the reported splat. I'll try to cook a patch tomorrow. Thanks, Paolo
diff --git a/include/net/sock.h b/include/net/sock.h index cce23ac4d514..7032009c0a94 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 9abc4fe25953..747d7e479d69 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 f891bc714668..0a89449d45f3 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -1049,13 +1049,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; @@ -1071,17 +1067,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 37ebcb7640eb..8190618d2771 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); @@ -3801,6 +3822,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 3b22313d1b86..44c2ca04132f 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -795,6 +795,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,