[PATCH AUTOSEL 4.19 573/671] ipv6: Handle race in addrconf_dad_work

From: David Ahern <redacted>

[ Upstream commit a3ce2a21bb8969ae27917281244fa91bf5f286d7 ]

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.

The 'dead' flag for an inet6_addr is set only under rtnl, in
addrconf_ifdown and it means the device is getting removed (or IPv6 is
disabled). 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. Based on that the existing dead + IF_READY check
can be moved to right after the rtnl_lock in addrconf_dad_work.

Fixes: f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Reported-by: Rajendra Dendukuri <redacted>
Signed-off-by: David Ahern <redacted>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 net/ipv6/addrconf.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ef309a26aba0..4cb94b5a9d39 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -3978,6 +3978,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;

@@ -4023,11 +4029,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);

-- 
2.20.1

On 1/16/20 10:18 AM, David Ahern wrote:

On 1/16/20 10:03 AM, Sasha Levin wrote:

quoted

From: David Ahern <redacted>

[ Upstream commit a3ce2a21bb8969ae27917281244fa91bf5f286d7 ]

That commit was reverted by 8ae72cbf62d2c1879456c0c5872f958e18f53711 and
then replaced by 2d819d250a1393a3e725715425ab70a0e0772a71

BTW, the AUTOSEL algorithm should be updated to look for reverts and
even ones that have already been nack'ed from a backport perspective.

I felt a bit of deja vu with my response and sure enough this patch was
selected back in October and I responded then that it should not be
backported.

On 1/16/20 10:03 AM, Sasha Levin wrote:

From: David Ahern <redacted>

[ Upstream commit a3ce2a21bb8969ae27917281244fa91bf5f286d7 ]

That commit was reverted by 8ae72cbf62d2c1879456c0c5872f958e18f53711 and
then replaced by 2d819d250a1393a3e725715425ab70a0e0772a71

On Thu, Jan 16, 2020 at 10:20:16AM -0700, David Ahern wrote:

On 1/16/20 10:18 AM, David Ahern wrote:

quoted

On 1/16/20 10:03 AM, Sasha Levin wrote:

quoted

From: David Ahern <redacted>

[ Upstream commit a3ce2a21bb8969ae27917281244fa91bf5f286d7 ]

That commit was reverted by 8ae72cbf62d2c1879456c0c5872f958e18f53711 and
then replaced by 2d819d250a1393a3e725715425ab70a0e0772a71

BTW, the AUTOSEL algorithm should be updated to look for reverts and
even ones that have already been nack'ed from a backport perspective.

I felt a bit of deja vu with my response and sure enough this patch was
selected back in October and I responded then that it should not be
backported.

Sorry about this David. This series is a result of an experimental work
I did rather than the regular AUTOSEL workflow, so it ended up
accidentally bubbling a few commits that were previously rejected.

-- 
Thanks,
Sasha