Re: [PATCH v2] resolve gtp possible deadlock warning

[Daniel Yang]

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?