On 10/7/24 2:54 PM, Daniel Yang wrote:
On Sat, Oct 5, 2024 at 12:25 AM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
On Sat, Oct 5, 2024 at 6:54 AM Daniel Yang <danielyangkang@xxxxxxxxx> wrote:
Fixes deadlock described in this bug:
https://syzkaller.appspot.com/bug?extid=e953a8f3071f5c0a28fd.
Specific crash report here:
https://syzkaller.appspot.com/text?tag=CrashReport&x=14670e07980000.
This bug is a false positive lockdep warning since gtp and smc use
completely different socket protocols.
Lockdep thinks that lock_sock() in smc will deadlock with gtp's
lock_sock() acquisition. Adding a function that initializes lockdep
labels for smc socks resolved the false positives in lockdep upon
testing. Since smc uses AF_SMC and SOCKSTREAM, two labels are created to
distinguish between proper smc socks and non smc socks incorrectly
input into the function.
Signed-off-by: Daniel Yang <danielyangkang@xxxxxxxxx>
Reported-by: syzbot+e953a8f3071f5c0a28fd@xxxxxxxxxxxxxxxxxxxxxxxxx
---
v1->v2: Add lockdep annotations instead of changing locking order
net/smc/af_smc.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 0316217b7..4de70bfd5 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -16,6 +16,8 @@
* based on prototype from Frank Blaschka
*/
+#include "linux/lockdep_types.h"
+#include "linux/socket.h"
#define KMSG_COMPONENT "smc"
#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
@@ -2755,6 +2757,24 @@ int smc_getname(struct socket *sock, struct sockaddr *addr,
return smc->clcsock->ops->getname(smc->clcsock, addr, peer);
}
+static struct lock_class_key smc_slock_key[2];
+static struct lock_class_key smc_key[2];
+
+static inline void smc_sock_lock_init(struct sock *sk)
+{
+ bool is_smc = (sk->sk_family == AF_SMC) && sk_is_tcp(sk);
+
+ sock_lock_init_class_and_name(sk,
+ is_smc ?
+ "smc_lock-AF_SMC_SOCKSTREAM" :
+ "smc_lock-INVALID",
+ &smc_slock_key[is_smc],
+ is_smc ?
+ "smc_sk_lock-AF_SMC_SOCKSTREAM" :
+ "smc_sk_lock-INVALID",
+ &smc_key[is_smc]);
+}
+
int smc_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
{
struct sock *sk = sock->sk;
@@ -2762,6 +2782,7 @@ int smc_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
int rc;
smc = smc_sk(sk);
+ smc_sock_lock_init(sk);
lock_sock(sk);
/* SMC does not support connect with fastopen */
--
2.39.2
sock_lock_init_class_and_name() is not meant to be repeatedly called,
from sendmsg()
Find a way to do this once, perhaps in smc_create_clcsk(), but I will
let SMC experts chime in.
So I tried putting the lockdep annotations in smc_create_clcsk() as
well as smc_sock_alloc() and they both fail to remove the false
positive but putting the annotations in smc_sendmsg() gets rid of
them. I put some print statements in the functions to see the
addresses of the socks.
[ 78.121827][ T8326] smc: smc_create_clcsk clcsk_addr: ffffc90007f0fd20
[ 78.122436][ T8326] smc: sendmsg sk_addr: ffffc90007f0fa88
[ 78.126907][ T8326] smc: __smc_create input_param clcsock: 0000000000000000
[ 78.134395][ T8326] smc: smc_sock_alloc sk_addr: ffffc90007f0fd70
[ 78.136679][ T8326] smc: smc_create_clcsk clcsk_clcsk: ffffc90007f0fd70
It appears that none of the smc allocation methods are called, so
where else exactly could the sock used in sendmsg be created?
I think the problem you described can be solved through
https://lore.kernel.org/netdev/20240912000446.1025844-1-xiyou.wangcong@xxxxxxxxx/, but Cong Wang
seems to have given up on following up at the moment. If you are interested, you can try take on
this problem.
Additionally, if you want to make sock_lock_init_class_and_name as a solution, the correct approach
might be (But I do not recommend doing so. I still hope to maintain consistency between IPPROTO_SMC
and other inet implementations as much as possible.)
+static struct lock_class_key smc_slock_keys[2];
+static struct lock_class_key smc_keys[2];
+
static int smc_inet_init_sock(struct sock *sk)
{
struct net *net = sock_net(sk);
+ int rc;
/* init common smc sock */
smc_sk_init(net, sk, IPPROTO_SMC);
/* create clcsock */
- return smc_create_clcsk(net, sk, sk->sk_family);
+ rc = smc_create_clcsk(net, sk, sk->sk_family);
+ if (rc)
+ return rc;
+
+ switch (sk->sk_family) {
+ case AF_INET:
+ sock_lock_init_class_and_name(sk, "slock-AF_INET-SMC",
+ &smc_slock_keys[0],
+ "sk_lock-AF_INET-SMC",
+ &smc_keys[0]);
+ break;
+ case AF_INET6:
+ sock_lock_init_class_and_name(sk, "slock-AF_INET6-SMC",
+ &smc_slock_keys[1],
+ "sk_lock-AF_INET6-SMC",
+ &smc_keys[1]);
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ }
+
+ return 0;
}
