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(-)
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
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.
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.
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.
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/10762126900 Initiator: Patchew Applier Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e76cf58756cd Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=888144 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)
© 2016 - 2024 Red Hat, Inc.