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