netdev - Re: [PATCH]ipv6: multicast: In mld_send_cr function moving read lock to second for loop

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]

Date:   Tue, 18 Sep 2018 18:51:39 +0530
From:   Guruswamy Basavaiah <guru2018@...il.com>
To:     davem@...emloft.net
Cc:     netdev@...r.kernel.org, Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH]ipv6: multicast: In mld_send_cr function moving read lock
 to second for loop

> This will lead to deadlocks, idev->mc_lock must be taken with _bh().
Modified the existing spin_lock to spin_lock_bh

> I have zero confidence in this change, did you do any stress testing
> with lockdep enabled?  It would have caught this quickly.

With LOCKDEP enabled ran  LTP multicast stress with the below new patch.
Test case is successful and LOCKDEP did not catch any dead lock issues.

---
>From 789840a6c6f783311ea7dfd832787c27d5b8359f Mon Sep 17 00:00:00 2001
From: Guruswamy Basavaiah <guru2018@...il.com>
Date: Tue, 18 Sep 2018 18:40:21 +0530
Subject: [PATCH] ipv6: multicast: In mld_send_cr function moving read lock to
 second for loop

In function mld_send_cr, the first loop is already protected by
idev->mc_lock, it dont need idev->lock read lock, hence moving it
only to second for loop. And converting the existing spin_lock to
spin_lock_bh

Signed-off-by: Guruswamy Basavaiah <guru2018@...il.com>
---
 net/ipv6/mcast.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 4ae54aaca373..751e580eb0ed 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1912,8 +1912,7 @@ static void mld_send_cr(struct inet6_dev *idev)
     struct sk_buff *skb = NULL;
     int type, dtype;

-    read_lock_bh(&idev->lock);
-    spin_lock(&idev->mc_lock);
+    spin_lock_bh(&idev->mc_lock);

     /* deleted MCA's */
     pmc_prev = NULL;
@@ -1947,8 +1946,9 @@ static void mld_send_cr(struct inet6_dev *idev)
         } else
             pmc_prev = pmc;
     }
-    spin_unlock(&idev->mc_lock);
+    spin_unlock_bh(&idev->mc_lock);

+    read_lock_bh(&idev->lock);
     /* change recs */
     for (pmc = idev->mc_list; pmc; pmc = pmc->next) {
         spin_lock_bh(&pmc->mca_lock);
-- 
2.14.4
On Sun, 19 Aug 2018 at 02:28, David Miller <davem@...emloft.net> wrote:
>
> From: Guruswamy Basavaiah <guru2018@...il.com>
> Date: Fri, 17 Aug 2018 18:01:41 +0530
>
> > @@ -1860,7 +1860,6 @@ static void mld_send_cr(struct inet6_dev *idev)
> >      struct sk_buff *skb = NULL;
> >      int type, dtype;
> >
> > -    read_lock_bh(&idev->lock);
> >      spin_lock(&idev->mc_lock);
> >
> >      /* deleted MCA's */
>
> This will lead to deadlocks, idev->mc_lock must be taken with _bh().
>
> I have zero confidence in this change, did you do any stress testing
> with lockdep enabled?  It would have caught this quickly.



-- 
Guruswamy Basavaiah