* [PATCH net] ipv6: addrconf: Fix recursive spin lock call
@ 2016-02-01 19:13 subashab
2016-02-01 19:37 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: subashab @ 2016-02-01 19:13 UTC (permalink / raw)
To: Netdev; +Cc: 'Eric Dumazet', Hannes Frederic Sowa
A rcu stall with the following backtrace was seen on a system with
forwarding, optimistic_dad and use_optimistic set. To reproduce,
set these flags and then start ipv6 autoconf.
This occurs because the device write_lock is acquired while already
holding the read_lock. Back trace below -
INFO: rcu_preempt self-detected stall on CPU { 1} (t=2100 jiffies
g=3992 c=3991 q=4471)
<6> Task dump for CPU 1:
<2> kworker/1:0 R running task 12168 15 2 0x00000002
<2> Workqueue: ipv6_addrconf addrconf_dad_work
<6> Call trace:
<2> [<ffffffc000084da8>] el1_irq+0x68/0xdc
<2> [<ffffffc000cc4e0c>] _raw_write_lock_bh+0x20/0x30
<2> [<ffffffc000bc5dd8>] __ipv6_dev_ac_inc+0x64/0x1b4
<2> [<ffffffc000bcbd2c>] addrconf_join_anycast+0x9c/0xc4
<2> [<ffffffc000bcf9f0>] __ipv6_ifa_notify+0x160/0x29c
<2> [<ffffffc000bcfb7c>] ipv6_ifa_notify+0x50/0x70
<2> [<ffffffc000bd035c>] addrconf_dad_work+0x314/0x334
<2> [<ffffffc0000b64c8>] process_one_work+0x244/0x3fc
<2> [<ffffffc0000b7324>] worker_thread+0x2f8/0x418
<2> [<ffffffc0000bb40c>] kthread+0xe0/0xec
Change-Id: I270c05598622d400b178d758fdbd8296cf521ee8
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
net/ipv6/addrconf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 2163871..ae8ac1a 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3301,7 +3301,9 @@ static void addrconf_dad_begin(struct inet6_ifaddr
*ifp)
/* Because optimistic nodes can use this address,
* notify listeners. If DAD fails, RTM_DELADDR is sent.
*/
+ read_unlock_bh(&idev->lock);
ipv6_ifa_notify(RTM_NEWADDR, ifp);
+ read_lock_bh(&idev->lock);
}
}
--
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] ipv6: addrconf: Fix recursive spin lock call
2016-02-01 19:13 [PATCH net] ipv6: addrconf: Fix recursive spin lock call subashab
@ 2016-02-01 19:37 ` Eric Dumazet
2016-02-01 19:46 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2016-02-01 19:37 UTC (permalink / raw)
To: subashab, Erik Kline; +Cc: Netdev, Hannes Frederic Sowa
On Mon, 2016-02-01 at 19:13 +0000, subashab@codeaurora.org wrote:
> A rcu stall with the following backtrace was seen on a system with
> forwarding, optimistic_dad and use_optimistic set. To reproduce,
> set these flags and then start ipv6 autoconf.
>
> This occurs because the device write_lock is acquired while already
> holding the read_lock. Back trace below -
>
> INFO: rcu_preempt self-detected stall on CPU { 1} (t=2100 jiffies
> g=3992 c=3991 q=4471)
> <6> Task dump for CPU 1:
> <2> kworker/1:0 R running task 12168 15 2 0x00000002
> <2> Workqueue: ipv6_addrconf addrconf_dad_work
> <6> Call trace:
> <2> [<ffffffc000084da8>] el1_irq+0x68/0xdc
> <2> [<ffffffc000cc4e0c>] _raw_write_lock_bh+0x20/0x30
> <2> [<ffffffc000bc5dd8>] __ipv6_dev_ac_inc+0x64/0x1b4
> <2> [<ffffffc000bcbd2c>] addrconf_join_anycast+0x9c/0xc4
> <2> [<ffffffc000bcf9f0>] __ipv6_ifa_notify+0x160/0x29c
> <2> [<ffffffc000bcfb7c>] ipv6_ifa_notify+0x50/0x70
> <2> [<ffffffc000bd035c>] addrconf_dad_work+0x314/0x334
> <2> [<ffffffc0000b64c8>] process_one_work+0x244/0x3fc
> <2> [<ffffffc0000b7324>] worker_thread+0x2f8/0x418
> <2> [<ffffffc0000bb40c>] kthread+0xe0/0xec
>
> Change-Id: I270c05598622d400b178d758fdbd8296cf521ee8
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
> net/ipv6/addrconf.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 2163871..ae8ac1a 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3301,7 +3301,9 @@ static void addrconf_dad_begin(struct inet6_ifaddr
> *ifp)
> /* Because optimistic nodes can use this address,
> * notify listeners. If DAD fails, RTM_DELADDR is sent.
> */
> + read_unlock_bh(&idev->lock);
> ipv6_ifa_notify(RTM_NEWADDR, ifp);
> + read_lock_bh(&idev->lock);
> }
> }
>
Hi
1) Thanks for looking at this problem ;)
2) Your patch was mangled.
3) Seems to lack 'Fixes: 7fd2561e4ebdd ("net: ipv6: Add a sysctl to make
optimistic addresses useful candidates") tag
4) You should CC patch author, Erik Kline (I did that)
5) releasing the lock as you did is probably racy.
I would rather try :
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 38eeddedfc21..d6b7ab07f914 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3538,6 +3538,7 @@ static void addrconf_dad_begin(struct inet6_ifaddr *ifp)
{
struct inet6_dev *idev = ifp->idev;
struct net_device *dev = idev->dev;
+ bool notify = false;
addrconf_join_solict(dev, &ifp->addr);
@@ -3583,7 +3584,8 @@ static void addrconf_dad_begin(struct inet6_ifaddr *ifp)
/* Because optimistic nodes can use this address,
* notify listeners. If DAD fails, RTM_DELADDR is sent.
*/
- ipv6_ifa_notify(RTM_NEWADDR, ifp);
+ notify = true;
+ in6_ifa_hold(ifp);
}
}
@@ -3591,6 +3593,10 @@ static void addrconf_dad_begin(struct inet6_ifaddr *ifp)
out:
spin_unlock(&ifp->lock);
read_unlock_bh(&idev->lock);
+ if (notify) {
+ ipv6_ifa_notify(RTM_NEWADDR, ifp);
+ in6_ifa_put(ifp);
+ }
}
static void addrconf_dad_start(struct inet6_ifaddr *ifp)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] ipv6: addrconf: Fix recursive spin lock call
2016-02-01 19:37 ` Eric Dumazet
@ 2016-02-01 19:46 ` Eric Dumazet
2016-02-02 0:07 ` subashab
0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2016-02-01 19:46 UTC (permalink / raw)
To: subashab; +Cc: Erik Kline, Netdev, Hannes Frederic Sowa
On Mon, 2016-02-01 at 11:37 -0800, Eric Dumazet wrote:
> I would rather try :
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 38eeddedfc21..d6b7ab07f914 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3538,6 +3538,7 @@ static void addrconf_dad_begin(struct inet6_ifaddr *ifp)
> {
> struct inet6_dev *idev = ifp->idev;
> struct net_device *dev = idev->dev;
> + bool notify = false;
>
> addrconf_join_solict(dev, &ifp->addr);
>
> @@ -3583,7 +3584,8 @@ static void addrconf_dad_begin(struct inet6_ifaddr *ifp)
> /* Because optimistic nodes can use this address,
> * notify listeners. If DAD fails, RTM_DELADDR is sent.
> */
> - ipv6_ifa_notify(RTM_NEWADDR, ifp);
> + notify = true;
> + in6_ifa_hold(ifp);
Actually the in6_ifa_hold() is not needed.
> }
> }
>
> @@ -3591,6 +3593,10 @@ static void addrconf_dad_begin(struct inet6_ifaddr *ifp)
> out:
> spin_unlock(&ifp->lock);
> read_unlock_bh(&idev->lock);
> + if (notify) {
> + ipv6_ifa_notify(RTM_NEWADDR, ifp);
> + in6_ifa_put(ifp);
And in6_ifa_put() not needed once in6_ifa_hold() is removed.
> + }
> }
>
> static void addrconf_dad_start(struct inet6_ifaddr *ifp)
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] ipv6: addrconf: Fix recursive spin lock call
2016-02-01 19:46 ` Eric Dumazet
@ 2016-02-02 0:07 ` subashab
0 siblings, 0 replies; 4+ messages in thread
From: subashab @ 2016-02-02 0:07 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Erik Kline, Netdev, Hannes Frederic Sowa
> On Mon, 2016-02-01 at 11:37 -0800, Eric Dumazet wrote:
>
>> I would rather try :
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 38eeddedfc21..d6b7ab07f914 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -3538,6 +3538,7 @@ static void addrconf_dad_begin(struct inet6_ifaddr
>> *ifp)
>> {
>> struct inet6_dev *idev = ifp->idev;
>> struct net_device *dev = idev->dev;
>> + bool notify = false;
>>
>> addrconf_join_solict(dev, &ifp->addr);
>>
>> @@ -3583,7 +3584,8 @@ static void addrconf_dad_begin(struct inet6_ifaddr
>> *ifp)
>> /* Because optimistic nodes can use this address,
>> * notify listeners. If DAD fails, RTM_DELADDR is sent.
>> */
>> - ipv6_ifa_notify(RTM_NEWADDR, ifp);
>> + notify = true;
>> + in6_ifa_hold(ifp);
>
> Actually the in6_ifa_hold() is not needed.
>
>> }
>> }
>>
>> @@ -3591,6 +3593,10 @@ static void addrconf_dad_begin(struct
>> inet6_ifaddr *ifp)
>> out:
>> spin_unlock(&ifp->lock);
>> read_unlock_bh(&idev->lock);
>> + if (notify) {
>> + ipv6_ifa_notify(RTM_NEWADDR, ifp);
>> + in6_ifa_put(ifp);
>
> And in6_ifa_put() not needed once in6_ifa_hold() is removed.
>
>> + }
>> }
>>
>> static void addrconf_dad_start(struct inet6_ifaddr *ifp)
>>
>>
Thanks Eric. I tested the scenario with your suggestion and I don't see a
RCU stall now. I will send out v2 of this patch.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-02 0:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-01 19:13 [PATCH net] ipv6: addrconf: Fix recursive spin lock call subashab
2016-02-01 19:37 ` Eric Dumazet
2016-02-01 19:46 ` Eric Dumazet
2016-02-02 0:07 ` subashab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).