* [PATCH net] ipv6: Handle race in addrconf_dad_work
@ 2019-10-01 1:37 David Ahern
2019-10-01 2:01 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2019-10-01 1:37 UTC (permalink / raw)
To: davem, jakub.kicinski; +Cc: netdev, David Ahern
From: David Ahern <dsahern@gmail.com>
Rajendra reported a kernel panic when a link was taken down:
[ 6870.263084] BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8
[ 6870.271856] IP: [<ffffffff8efc5764>] __ipv6_ifa_notify+0x154/0x290
<snip>
[ 6870.570501] Call Trace:
[ 6870.573238] [<ffffffff8efc58c6>] ? ipv6_ifa_notify+0x26/0x40
[ 6870.579665] [<ffffffff8efc98ec>] ? addrconf_dad_completed+0x4c/0x2c0
[ 6870.586869] [<ffffffff8efe70c6>] ? ipv6_dev_mc_inc+0x196/0x260
[ 6870.593491] [<ffffffff8efc9c6a>] ? addrconf_dad_work+0x10a/0x430
[ 6870.600305] [<ffffffff8f01ade4>] ? __switch_to_asm+0x34/0x70
[ 6870.606732] [<ffffffff8ea93a7a>] ? process_one_work+0x18a/0x430
[ 6870.613449] [<ffffffff8ea93d6d>] ? worker_thread+0x4d/0x490
[ 6870.619778] [<ffffffff8ea93d20>] ? process_one_work+0x430/0x430
[ 6870.626495] [<ffffffff8ea99dd9>] ? kthread+0xd9/0xf0
[ 6870.632145] [<ffffffff8f01ade4>] ? __switch_to_asm+0x34/0x70
[ 6870.638573] [<ffffffff8ea99d00>] ? kthread_park+0x60/0x60
[ 6870.644707] [<ffffffff8f01ae77>] ? ret_from_fork+0x57/0x70
[ 6870.650936] Code: 31 c0 31 d2 41 b9 20 00 08 02 b9 09 00 00 0
addrconf_dad_work is kicked to be scheduled when a device is brought
up. There is a race between addrcond_dad_work getting scheduled and
taking the rtnl lock and a process taking the link down (under rtnl).
The latter removes the host route from the inet6_addr as part of
addrconf_ifdown which is run for NETDEV_DOWN. The former attempts
to use the host route in ipv6_ifa_notify. If the down event removes
the host route due to the race to the rtnl, then the BUG listed above
occurs.
This scenario does not occur when the ipv6 address is not kept
(net.ipv6.conf.all.keep_addr_on_down = 0) as addrconf_ifdown sets the
state of the ifp to DEAD. Handle when the addresses are kept by checking
IF_READY which is reset by addrconf_ifdown.
Fixes: f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Reported-by: Rajendra Dendukuri <rajendra.dendukuri@broadcom.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/ipv6/addrconf.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6a576ff92c39..e2759ef73b03 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4032,6 +4032,12 @@ static void addrconf_dad_work(struct work_struct *w)
rtnl_lock();
+ /* check if device was taken down before this delayed work
+ * function could be canceled
+ */
+ if (!(idev->if_flags & IF_READY))
+ goto out;
+
spin_lock_bh(&ifp->lock);
if (ifp->state == INET6_IFADDR_STATE_PREDAD) {
action = DAD_BEGIN;
--
2.11.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] ipv6: Handle race in addrconf_dad_work
2019-10-01 1:37 [PATCH net] ipv6: Handle race in addrconf_dad_work David Ahern
@ 2019-10-01 2:01 ` Eric Dumazet
2019-10-01 2:23 ` David Ahern
0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2019-10-01 2:01 UTC (permalink / raw)
To: David Ahern, davem, jakub.kicinski; +Cc: netdev, David Ahern
On 9/30/19 6:37 PM, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Rajendra reported a kernel panic when a link was taken down:
>
> [ 6870.263084] BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8
> [ 6870.271856] IP: [<ffffffff8efc5764>] __ipv6_ifa_notify+0x154/0x290
>
> <snip>
>
> [ 6870.570501] Call Trace:
> [ 6870.573238] [<ffffffff8efc58c6>] ? ipv6_ifa_notify+0x26/0x40
> [ 6870.579665] [<ffffffff8efc98ec>] ? addrconf_dad_completed+0x4c/0x2c0
> [ 6870.586869] [<ffffffff8efe70c6>] ? ipv6_dev_mc_inc+0x196/0x260
> [ 6870.593491] [<ffffffff8efc9c6a>] ? addrconf_dad_work+0x10a/0x430
> [ 6870.600305] [<ffffffff8f01ade4>] ? __switch_to_asm+0x34/0x70
> [ 6870.606732] [<ffffffff8ea93a7a>] ? process_one_work+0x18a/0x430
> [ 6870.613449] [<ffffffff8ea93d6d>] ? worker_thread+0x4d/0x490
> [ 6870.619778] [<ffffffff8ea93d20>] ? process_one_work+0x430/0x430
> [ 6870.626495] [<ffffffff8ea99dd9>] ? kthread+0xd9/0xf0
> [ 6870.632145] [<ffffffff8f01ade4>] ? __switch_to_asm+0x34/0x70
> [ 6870.638573] [<ffffffff8ea99d00>] ? kthread_park+0x60/0x60
> [ 6870.644707] [<ffffffff8f01ae77>] ? ret_from_fork+0x57/0x70
> [ 6870.650936] Code: 31 c0 31 d2 41 b9 20 00 08 02 b9 09 00 00 0
>
> addrconf_dad_work is kicked to be scheduled when a device is brought
> up. There is a race between addrcond_dad_work getting scheduled and
> taking the rtnl lock and a process taking the link down (under rtnl).
> The latter removes the host route from the inet6_addr as part of
> addrconf_ifdown which is run for NETDEV_DOWN. The former attempts
> to use the host route in ipv6_ifa_notify. If the down event removes
> the host route due to the race to the rtnl, then the BUG listed above
> occurs.
>
> This scenario does not occur when the ipv6 address is not kept
> (net.ipv6.conf.all.keep_addr_on_down = 0) as addrconf_ifdown sets the
> state of the ifp to DEAD. Handle when the addresses are kept by checking
> IF_READY which is reset by addrconf_ifdown.
>
> Fixes: f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
> Reported-by: Rajendra Dendukuri <rajendra.dendukuri@broadcom.com>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> net/ipv6/addrconf.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 6a576ff92c39..e2759ef73b03 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4032,6 +4032,12 @@ static void addrconf_dad_work(struct work_struct *w)
>
> rtnl_lock();
>
> + /* check if device was taken down before this delayed work
> + * function could be canceled
> + */
> + if (!(idev->if_flags & IF_READY))
> + goto out;
> +
> spin_lock_bh(&ifp->lock);
> if (ifp->state == INET6_IFADDR_STATE_PREDAD) {
> action = DAD_BEGIN;
>
Do we need to keep the test on IF_READY done later in this function ?
If IF_READY can disappear only under RTNL, we might clean this.
(unless addrconf_dad_work() releases rtnl and reacquires it)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6a576ff92c399fbb686c839c525e700f2579b1a9..64e87e26187e983ad5da9f7496993202d63daa06 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4077,7 +4077,7 @@ static void addrconf_dad_work(struct work_struct *w)
goto out;
write_lock_bh(&idev->lock);
- if (idev->dead || !(idev->if_flags & IF_READY)) {
+ if (idev->dead) {
write_unlock_bh(&idev->lock);
goto out;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] ipv6: Handle race in addrconf_dad_work
2019-10-01 2:01 ` Eric Dumazet
@ 2019-10-01 2:23 ` David Ahern
2019-10-01 2:40 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2019-10-01 2:23 UTC (permalink / raw)
To: Eric Dumazet, David Ahern, davem, jakub.kicinski; +Cc: netdev
On 9/30/19 8:01 PM, Eric Dumazet wrote:
>
>
> On 9/30/19 6:37 PM, David Ahern wrote:
>> From: David Ahern <dsahern@gmail.com>
>>
>> Rajendra reported a kernel panic when a link was taken down:
>>
>> [ 6870.263084] BUG: unable to handle kernel NULL pointer dereference at 00000000000000a8
>> [ 6870.271856] IP: [<ffffffff8efc5764>] __ipv6_ifa_notify+0x154/0x290
>>
>> <snip>
>>
>> [ 6870.570501] Call Trace:
>> [ 6870.573238] [<ffffffff8efc58c6>] ? ipv6_ifa_notify+0x26/0x40
>> [ 6870.579665] [<ffffffff8efc98ec>] ? addrconf_dad_completed+0x4c/0x2c0
>> [ 6870.586869] [<ffffffff8efe70c6>] ? ipv6_dev_mc_inc+0x196/0x260
>> [ 6870.593491] [<ffffffff8efc9c6a>] ? addrconf_dad_work+0x10a/0x430
>> [ 6870.600305] [<ffffffff8f01ade4>] ? __switch_to_asm+0x34/0x70
>> [ 6870.606732] [<ffffffff8ea93a7a>] ? process_one_work+0x18a/0x430
>> [ 6870.613449] [<ffffffff8ea93d6d>] ? worker_thread+0x4d/0x490
>> [ 6870.619778] [<ffffffff8ea93d20>] ? process_one_work+0x430/0x430
>> [ 6870.626495] [<ffffffff8ea99dd9>] ? kthread+0xd9/0xf0
>> [ 6870.632145] [<ffffffff8f01ade4>] ? __switch_to_asm+0x34/0x70
>> [ 6870.638573] [<ffffffff8ea99d00>] ? kthread_park+0x60/0x60
>> [ 6870.644707] [<ffffffff8f01ae77>] ? ret_from_fork+0x57/0x70
>> [ 6870.650936] Code: 31 c0 31 d2 41 b9 20 00 08 02 b9 09 00 00 0
>>
>> addrconf_dad_work is kicked to be scheduled when a device is brought
>> up. There is a race between addrcond_dad_work getting scheduled and
>> taking the rtnl lock and a process taking the link down (under rtnl).
>> The latter removes the host route from the inet6_addr as part of
>> addrconf_ifdown which is run for NETDEV_DOWN. The former attempts
>> to use the host route in ipv6_ifa_notify. If the down event removes
>> the host route due to the race to the rtnl, then the BUG listed above
>> occurs.
>>
>> This scenario does not occur when the ipv6 address is not kept
>> (net.ipv6.conf.all.keep_addr_on_down = 0) as addrconf_ifdown sets the
>> state of the ifp to DEAD. Handle when the addresses are kept by checking
>> IF_READY which is reset by addrconf_ifdown.
>>
>> Fixes: f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
>> Reported-by: Rajendra Dendukuri <rajendra.dendukuri@broadcom.com>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> ---
>> net/ipv6/addrconf.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 6a576ff92c39..e2759ef73b03 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -4032,6 +4032,12 @@ static void addrconf_dad_work(struct work_struct *w)
>>
>> rtnl_lock();
>>
>> + /* check if device was taken down before this delayed work
>> + * function could be canceled
>> + */
>> + if (!(idev->if_flags & IF_READY))
>> + goto out;
>> +
>> spin_lock_bh(&ifp->lock);
>> if (ifp->state == INET6_IFADDR_STATE_PREDAD) {
>> action = DAD_BEGIN;
>>
>
>
> Do we need to keep the test on IF_READY done later in this function ?
>
> If IF_READY can disappear only under RTNL, we might clean this.
>
> (unless addrconf_dad_work() releases rtnl and reacquires it)
Unless I am missing something none of the functions called by dad_work
release the rtnl, but your comment did have me second guessing the locking.
The interesting cases for changing the idev flag are addrconf_notify
(NETDEV_UP and NETDEV_CHANGE) and addrconf_ifdown (reset the flag). The
former does not have the idev lock - only rtnl. The latter has both.
Checking the flag is inconsistent with respect to locks.
As for your suggestion, the 'dead' flag is set only under rtnl in
addrconf_ifdown and it means the device is getting removed (or IPv6 is
disabled). Based on that I think the existing:
if (idev->dead || !(idev->if_flags & IF_READY))
goto out;
can be moved to right after the rtnl_lock in addrconf_dad_work in place
of the above change, so the end result is:
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 6a576ff92c39..dd3be06d5a06 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4032,6 +4032,12 @@ static void addrconf_dad_work(struct work_struct *w)
rtnl_lock();
+ /* check if device was taken down before this delayed work
+ * function could be canceled
+ */
+ if (idev->dead || !(idev->if_flags & IF_READY))
+ goto out;
+
spin_lock_bh(&ifp->lock);
if (ifp->state == INET6_IFADDR_STATE_PREDAD) {
action = DAD_BEGIN;
@@ -4077,11 +4083,6 @@ static void addrconf_dad_work(struct work_struct *w)
goto out;
write_lock_bh(&idev->lock);
- if (idev->dead || !(idev->if_flags & IF_READY)) {
- write_unlock_bh(&idev->lock);
- goto out;
- }
-
spin_lock(&ifp->lock);
if (ifp->state == INET6_IFADDR_STATE_DEAD) {
spin_unlock(&ifp->lock);
agree?
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] ipv6: Handle race in addrconf_dad_work
2019-10-01 2:23 ` David Ahern
@ 2019-10-01 2:40 ` Eric Dumazet
0 siblings, 0 replies; 4+ messages in thread
From: Eric Dumazet @ 2019-10-01 2:40 UTC (permalink / raw)
To: David Ahern, Eric Dumazet, David Ahern, davem, jakub.kicinski; +Cc: netdev
On 9/30/19 7:23 PM, David Ahern wrote:
> On 9/30/19 8:01 PM, Eric Dumazet wrote:
>>
>>
>> Do we need to keep the test on IF_READY done later in this function ?
>>
>> If IF_READY can disappear only under RTNL, we might clean this.
>>
>> (unless addrconf_dad_work() releases rtnl and reacquires it)
>
> Unless I am missing something none of the functions called by dad_work
> release the rtnl, but your comment did have me second guessing the locking.
>
> The interesting cases for changing the idev flag are addrconf_notify
> (NETDEV_UP and NETDEV_CHANGE) and addrconf_ifdown (reset the flag). The
> former does not have the idev lock - only rtnl. The latter has both.
> Checking the flag is inconsistent with respect to locks.
>
> As for your suggestion, the 'dead' flag is set only under rtnl in
> addrconf_ifdown and it means the device is getting removed (or IPv6 is
> disabled). Based on that I think the existing:
>
> if (idev->dead || !(idev->if_flags & IF_READY))
> goto out;
>
> can be moved to right after the rtnl_lock in addrconf_dad_work in place
> of the above change, so the end result is:
>
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 6a576ff92c39..dd3be06d5a06 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4032,6 +4032,12 @@ static void addrconf_dad_work(struct work_struct *w)
>
> rtnl_lock();
>
> + /* check if device was taken down before this delayed work
> + * function could be canceled
> + */
> + if (idev->dead || !(idev->if_flags & IF_READY))
> + goto out;
> +
> spin_lock_bh(&ifp->lock);
> if (ifp->state == INET6_IFADDR_STATE_PREDAD) {
> action = DAD_BEGIN;
> @@ -4077,11 +4083,6 @@ static void addrconf_dad_work(struct work_struct *w)
> goto out;
>
> write_lock_bh(&idev->lock);
> - if (idev->dead || !(idev->if_flags & IF_READY)) {
> - write_unlock_bh(&idev->lock);
> - goto out;
> - }
> -
> spin_lock(&ifp->lock);
> if (ifp->state == INET6_IFADDR_STATE_DEAD) {
> spin_unlock(&ifp->lock);
>
>
> agree?
>
SGTM, thanks !
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-01 2:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-01 1:37 [PATCH net] ipv6: Handle race in addrconf_dad_work David Ahern
2019-10-01 2:01 ` Eric Dumazet
2019-10-01 2:23 ` David Ahern
2019-10-01 2:40 ` Eric Dumazet
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).