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] [day] [month] [year] [list]

Message-ID: <CANn89iKk8TOvzD4cAanACtD0-x2pciEoSJbk9mF97wxNzxmUCg@mail.gmail.com>
Date: Sat, 5 Oct 2024 09:24:48 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Daniel Yang <danielyangkang@...il.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 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.