lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 5 Sep 2017 10:12:20 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Huang, Ying" <ying.huang@...el.com>
cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH -v2] IRQ, cpu-hotplug: Fix a race between CPU hotplug
 and IRQ desc alloc/free

On Tue, 5 Sep 2017, Huang, Ying wrote:

> From: Huang Ying <ying.huang@...el.com>
> 
> When developing code to bootup some APs (Application CPUs)
> asynchronously, the following kernel panic is encountered.  After
> checking the code, it is found that the irq_to_desc() may return NULL
> during CPU hotplug.  So the NULL pointer checking is added to fix
> this.

You forgot to describe why this can happen. "After checking the code" is
not really helpful for someone who looks at that commit.

for_each_active_irq() is iterated with the sparse lock held in both cases
(cpu up and down). So if there is an active bit in the sparse map and the
the radix tree entry is empty then there is an inconsistency. The
inconsistency originates from the way the irq descriptor allocation/free is
implemented. The bitmap is set/cleared seperately from the actual pointer
store/remove in the radix tree:

The allocation side:

    irq_sparse_lock();
    bitmap_set();
    irq_sparse_unlock();

    desc = alloc();
    irq_sparse_lock();
    store_in_radix_tree(irq, desc);
    irq_sparse_unlock();

The deallocation side:

    irq_sparse_lock();
    store_in_radix_tree(irq, NULL);
    irq_sparse_unlock();

    irq_sparse_lock();
    bitmap_clear();
    irq_sparse_unlock();

So the real question is, whether we keep it that way and have the extra
checks all over the place or simply extend the protected sections in the
alloc/free path.

Untested patch below.

Thanks,

	tglx
8<----------------

kernel/irq/irqdesc.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Index: b/kernel/irq/irqdesc.c
===================================================================
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -421,10 +421,8 @@ static void free_desc(unsigned int irq)
 	 * The sysfs entry must be serialized against a concurrent
 	 * irq_sysfs_init() as well.
 	 */
-	mutex_lock(&sparse_irq_lock);
 	kobject_del(&desc->kobj);
 	delete_irq_desc(irq);
-	mutex_unlock(&sparse_irq_lock);
 
 	/*
 	 * We free the descriptor, masks and stat fields via RCU. That
@@ -462,20 +460,14 @@ static int alloc_descs(unsigned int star
 		desc = alloc_desc(start + i, node, flags, mask, owner);
 		if (!desc)
 			goto err;
-		mutex_lock(&sparse_irq_lock);
 		irq_insert_desc(start + i, desc);
 		irq_sysfs_add(start + i, desc);
-		mutex_unlock(&sparse_irq_lock);
 	}
 	return start;
 
 err:
 	for (i--; i >= 0; i--)
 		free_desc(start + i);
-
-	mutex_lock(&sparse_irq_lock);
-	bitmap_clear(allocated_irqs, start, cnt);
-	mutex_unlock(&sparse_irq_lock);
 	return -ENOMEM;
 }
 
@@ -670,10 +662,10 @@ void irq_free_descs(unsigned int from, u
 	if (from >= nr_irqs || (from + cnt) > nr_irqs)
 		return;
 
+	mutex_lock(&sparse_irq_lock);
 	for (i = 0; i < cnt; i++)
 		free_desc(from + i);
 
-	mutex_lock(&sparse_irq_lock);
 	bitmap_clear(allocated_irqs, from, cnt);
 	mutex_unlock(&sparse_irq_lock);
 }
@@ -727,10 +719,9 @@ int __ref
 		if (ret)
 			goto err;
 	}
-
-	bitmap_set(allocated_irqs, start, cnt);
-	mutex_unlock(&sparse_irq_lock);
-	return alloc_descs(start, cnt, node, affinity, owner);
+	ret = alloc_descs(start, cnt, node, affinity, owner);
+	if (ret >= 0)
+		bitmap_set(allocated_irqs, start, cnt);
 
 err:
 	mutex_unlock(&sparse_irq_lock);





Powered by blists - more mailing lists