[Patch net] mptcp: initialize sock lock with its own lockdep keys

[Patch net] mptcp: initialize sock lock with its own lockdep keys

[Cong Wang]

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 fix 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.

Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com
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>
---
 include/net/sock.h     |  1 +
 net/core/sock.c        |  2 ++
 net/mptcp/pm_netlink.c | 18 ++++++++++++------
 net/mptcp/protocol.c   |  7 +++++++
 net/mptcp/protocol.h   |  1 +
 5 files changed, 23 insertions(+), 6 deletions(-)

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..5f9f06180c67 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1052,10 +1052,20 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
 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;
+
+	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 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;
@@ -1077,11 +1087,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	 * 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..ce68ff4475d0 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,11 @@ static int mptcp_init_sock(struct sock *sk)
 	return 0;
 }
 
+static void mptcp_init_clone(struct sock *sk)
+{
+	mptcp_sock_lock_init(sk);
+}
+
 static void __mptcp_clear_xmit(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -3801,6 +3807,7 @@ static struct proto mptcp_prot = {
 	.name		= "MPTCP",
 	.owner		= THIS_MODULE,
 	.init		= mptcp_init_sock,
+	.init_clone	= mptcp_init_clone,
 	.connect	= mptcp_connect,
 	.disconnect	= mptcp_disconnect,
 	.close		= mptcp_close,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3b22313d1b86..457c01eac25f 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1135,6 +1135,7 @@ static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflo
 
 void __init mptcp_pm_nl_init(void);
 void mptcp_pm_nl_work(struct mptcp_sock *msk);
+void mptcp_sock_lock_init(struct sock *sk);
 unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);
-- 
2.34.1

Re: [Patch net] mptcp: initialize sock lock with its own lockdep keys

[Matthieu Baerts]

Hi Cong Wang,

On 08/09/2024 20:06, 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 fix 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.

Thank you for this patch!

The fix looks good to me, but I need to double-check if we can avoid
modifying the proto structure. Here is a first review.


From what I understand, it looks like syzbot reported a lockdep false
positive issue, right? In this case, can you clearly mention that in the
commit message, to avoid misinterpretations?

> Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com

checkpatch.pl reports that "Reported-by: should be immediately followed
by Closes: with a URL to the report".

Also, even if it is a false positive, it sounds better to consider this
as a fix, to avoid having new bug reports about that. In this case, can
you please add a "Fixes: <commit>" tag and a "Cc: stable" tag here please?

> Cc: Matthieu Baerts <matttbe@kernel.org>
> Cc: Mat Martineau <martineau@kernel.org>
> Cc: Geliang Tang <geliang@kernel.org>

(If a new version is needed here, feel free to remove the Netdev ML from
the CC list, and only add the MPTCP ML: we can apply this patch on MPTCP
side first, and send it to Netdev later, when it will be ready and
validated)

> Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> ---
>  include/net/sock.h     |  1 +
>  net/core/sock.c        |  2 ++
>  net/mptcp/pm_netlink.c | 18 ++++++++++++------
>  net/mptcp/protocol.c   |  7 +++++++
>  net/mptcp/protocol.h   |  1 +
>  5 files changed, 23 insertions(+), 6 deletions(-)
> 
> 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);

If the idea is to introduce a new ->init_clone(), should it not be
called ->lock_init() (or ->init_lock()) and replace the call to
sock_lock_init() when defined?

>  	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..5f9f06180c67 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1052,10 +1052,20 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
>  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)

If this helper is used by different parts in MPTCP, I think it would be
better to move it (and the associated keys) to protocol.c: such helper
is not specific to the Netlink path-manager, more to MPTCP in general.

> +{
> +	bool is_ipv6 = sk->sk_family == AF_INET6;
> +
> +	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]);

The alignment is not OK, and checkpatch.pl is complaining about that.
Can you keep the same indentation as it was before please?

> +}
> +
>  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;
> @@ -1077,11 +1087,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  	 * modifiers in several places, re-init the lock class for the msk
>  	 * socket to an mptcp specific one.
>  	 */

Please also move this comment above to the new mptcp_sock_lock_init()
function.

> -	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..ce68ff4475d0 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,11 @@ static int mptcp_init_sock(struct sock *sk)
>  	return 0;
>  }
>  
> +static void mptcp_init_clone(struct sock *sk)
> +{
> +	mptcp_sock_lock_init(sk);
> +}
> +
>  static void __mptcp_clear_xmit(struct sock *sk)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sk);
> @@ -3801,6 +3807,7 @@ static struct proto mptcp_prot = {
>  	.name		= "MPTCP",
>  	.owner		= THIS_MODULE,
>  	.init		= mptcp_init_sock,
> +	.init_clone	= mptcp_init_clone,

If 'mptcp_sock_lock_init()' is moved in this file, and 'init_clone' is
renamed to 'lock_init', maybe directly use 'mptcp_sock_lock_init' here?

>  	.connect	= mptcp_connect,
>  	.disconnect	= mptcp_disconnect,
>  	.close		= mptcp_close,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 3b22313d1b86..457c01eac25f 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1135,6 +1135,7 @@ static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflo
>  
>  void __init mptcp_pm_nl_init(void);
>  void mptcp_pm_nl_work(struct mptcp_sock *msk);
> +void mptcp_sock_lock_init(struct sock *sk);

(if the definition is moved to protocol.c, please also move it elsewhere
here, e.g. around mptcp_sk_clone_init())

>  unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [Patch net] mptcp: initialize sock lock with its own lockdep keys

[Cong Wang]

On Mon, Sep 09, 2024 at 05:03:32PM +0200, Matthieu Baerts wrote:
> Hi Cong Wang,
> 
> On 08/09/2024 20:06, 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 fix 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.
> 
> Thank you for this patch!
> 
> The fix looks good to me, but I need to double-check if we can avoid
> modifying the proto structure. Here is a first review.
> 
> 
> From what I understand, it looks like syzbot reported a lockdep false
> positive issue, right? In this case, can you clearly mention that in the
> commit message, to avoid misinterpretations?
> 
> > Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com
> 
> checkpatch.pl reports that "Reported-by: should be immediately followed
> by Closes: with a URL to the report".

Sure, didn't know this is helpful.

> 
> Also, even if it is a false positive, it sounds better to consider this
> as a fix, to avoid having new bug reports about that. In this case, can
> you please add a "Fixes: <commit>" tag and a "Cc: stable" tag here please?

I intended not to provide one because I don't think this needs to go to
-stable, it only fixes a lockdep warning instead of a real deadlock.
Please let me know if you prefer to target -stable.

> 
> > Cc: Matthieu Baerts <matttbe@kernel.org>
> > Cc: Mat Martineau <martineau@kernel.org>
> > Cc: Geliang Tang <geliang@kernel.org>
> 
> (If a new version is needed here, feel free to remove the Netdev ML from
> the CC list, and only add the MPTCP ML: we can apply this patch on MPTCP
> side first, and send it to Netdev later, when it will be ready and
> validated)

OK.

> 
> > Signed-off-by: Cong Wang <cong.wang@bytedance.com>
> > ---
> >  include/net/sock.h     |  1 +
> >  net/core/sock.c        |  2 ++
> >  net/mptcp/pm_netlink.c | 18 ++++++++++++------
> >  net/mptcp/protocol.c   |  7 +++++++
> >  net/mptcp/protocol.h   |  1 +
> >  5 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > 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);
> 
> If the idea is to introduce a new ->init_clone(), should it not be
> called ->lock_init() (or ->init_lock()) and replace the call to
> sock_lock_init() when defined?

'lock_init' or 'init_lock' reads like we are initalizing a lock. :)

> 
> >  	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..5f9f06180c67 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1052,10 +1052,20 @@ static int mptcp_pm_nl_append_new_local_addr(struct pm_nl_pernet *pernet,
> >  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)
> 
> If this helper is used by different parts in MPTCP, I think it would be
> better to move it (and the associated keys) to protocol.c: such helper
> is not specific to the Netlink path-manager, more to MPTCP in general.

Sure, if you don't mind more lines of changes.

> 
> > +{
> > +	bool is_ipv6 = sk->sk_family == AF_INET6;
> > +
> > +	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]);
> 
> The alignment is not OK, and checkpatch.pl is complaining about that.
> Can you keep the same indentation as it was before please?

Sure, sorry for missing this.

> 
> > +}
> > +
> >  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;
> > @@ -1077,11 +1087,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> >  	 * modifiers in several places, re-init the lock class for the msk
> >  	 * socket to an mptcp specific one.
> >  	 */
> 
> Please also move this comment above to the new mptcp_sock_lock_init()
> function.

OK.

> 
> > -	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..ce68ff4475d0 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,11 @@ static int mptcp_init_sock(struct sock *sk)
> >  	return 0;
> >  }
> >  
> > +static void mptcp_init_clone(struct sock *sk)
> > +{
> > +	mptcp_sock_lock_init(sk);
> > +}
> > +
> >  static void __mptcp_clear_xmit(struct sock *sk)
> >  {
> >  	struct mptcp_sock *msk = mptcp_sk(sk);
> > @@ -3801,6 +3807,7 @@ static struct proto mptcp_prot = {
> >  	.name		= "MPTCP",
> >  	.owner		= THIS_MODULE,
> >  	.init		= mptcp_init_sock,
> > +	.init_clone	= mptcp_init_clone,
> 
> If 'mptcp_sock_lock_init()' is moved in this file, and 'init_clone' is
> renamed to 'lock_init', maybe directly use 'mptcp_sock_lock_init' here?

Sounds better.

> 
> >  	.connect	= mptcp_connect,
> >  	.disconnect	= mptcp_disconnect,
> >  	.close		= mptcp_close,
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 3b22313d1b86..457c01eac25f 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -1135,6 +1135,7 @@ static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflo
> >  
> >  void __init mptcp_pm_nl_init(void);
> >  void mptcp_pm_nl_work(struct mptcp_sock *msk);
> > +void mptcp_sock_lock_init(struct sock *sk);
> 
> (if the definition is moved to protocol.c, please also move it elsewhere
> here, e.g. around mptcp_sk_clone_init())

Got it.

Thanks.

Re: [Patch net] mptcp: initialize sock lock with its own lockdep keys

[Matthieu Baerts]

Hi Cong Wang,

On 11/09/2024 05:34, Cong Wang wrote:
> On Mon, Sep 09, 2024 at 05:03:32PM +0200, Matthieu Baerts wrote:
>> Hi Cong Wang,
>>
>> On 08/09/2024 20:06, 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 fix 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.
>>
>> Thank you for this patch!
>>
>> The fix looks good to me, but I need to double-check if we can avoid
>> modifying the proto structure. Here is a first review.
>>
>>
>> From what I understand, it looks like syzbot reported a lockdep false
>> positive issue, right? In this case, can you clearly mention that in the
>> commit message, to avoid misinterpretations?
>>
>>> Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com
>>
>> checkpatch.pl reports that "Reported-by: should be immediately followed
>> by Closes: with a URL to the report".
> 
> Sure, didn't know this is helpful.

It is useful for the reviewers/devs to find more info about the issue,
and for other bots to mark a bug report as closed.

>> Also, even if it is a false positive, it sounds better to consider this
>> as a fix, to avoid having new bug reports about that. In this case, can
>> you please add a "Fixes: <commit>" tag and a "Cc: stable" tag here please?
> 
> I intended not to provide one because I don't think this needs to go to
> -stable, it only fixes a lockdep warning instead of a real deadlock.
> Please let me know if you prefer to target -stable.

Yes, it is useful. Because if it is not backported, it is likely we will
get this bug report again with stable versions. And such bug reports are
always taking time to analyse.

(...)

>>> 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);
>>
>> If the idea is to introduce a new ->init_clone(), should it not be
>> called ->lock_init() (or ->init_lock()) and replace the call to
>> sock_lock_init() when defined?
> 
> 'lock_init' or 'init_lock' reads like we are initalizing a lock. :)

If it is replacing sock_lock_init() call when ->init_lock is defined, it
will be about initializing a lock ;)

Thank you for the v2 (in MPTCP ML), I will continue the reviews there.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.