Re: [PATCH v2] resolve gtp possible deadlock warning

[Eric Dumazet]

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.