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

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

[Guruswamy Basavaiah]

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.

Signed-off-by: Guruswamy Basavaiah <guru2018@gmail.com>
---
 net/ipv6/mcast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index d64ee7e..d8e7e15 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -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 */
@@ -1897,6 +1896,7 @@ static void mld_send_cr(struct inet6_dev *idev)
     }
     spin_unlock(&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.9.5

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

[David Miller]

From: Guruswamy Basavaiah <guru2018@gmail.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.

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

[Guruswamy Basavaiah]

> 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@gmail.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@gmail.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@davemloft.net> wrote:
>
> From: Guruswamy Basavaiah <guru2018@gmail.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