netdev - Re: [PATCH v2] resolve gtp possible deadlock warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGiJo8RCXp8MqTPcPY4vyQAJCMhOStSApZzA5RcTq5BJgzXoeQ@mail.gmail.com>
Date: Sun, 6 Oct 2024 23:54:43 -0700
From: Daniel Yang <danielyangkang@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Wenjia Zhang <wenjia@...ux.ibm.com>, Jan Karcher <jaka@...ux.ibm.com>, 
	"D. Wythe" <alibuda@...ux.alibaba.com>, Tony Lu <tonylu@...ux.alibaba.com>, 
	Wen Gu <guwen@...ux.alibaba.com>, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, linux-s390@...r.kernel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzbot+e953a8f3071f5c0a28fd@...kaller.appspotmail.com
Subject: Re: [PATCH v2] resolve gtp possible deadlock warning

On Sat, Oct 5, 2024 at 12:25 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Sat, Oct 5, 2024 at 6:54 AM Daniel Yang <danielyangkang@...il.com> 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@...il.com>
> > Reported-by: syzbot+e953a8f3071f5c0a28fd@...kaller.appspotmail.com
> > ---
> > 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?