Re: [PATCH v2] resolve gtp possible deadlock warning

[Daniel Yang]

On Wed, Oct 9, 2024 at 12:20 AM D. Wythe <alibuda@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> 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;
>   }
>
>

I took a look at Cong Wang's patches and it seems that switching from
rtnl to rcu is unfeasible since gtp_encap_enable_socket is called by
rtnl_newlink_create through a function pointer and the rtnl_mutex
acquisition happens somewhere up the call stack. Since many other
newlink() functions rely on the assumption of rtnl lock being
acquired, it seems quite unfeasible to use this approach in this case.
I tried your suggested solution and it got rid of the bug. I made
small changes to it for readability and will resend it. Should lockdep
annotations also be added to smc_sock_alloc since smc_sk_init() is
also called there as well or is it guaranteed to have non-conflicting
lockdep labels?