lkml.org 
[lkml]   [2017]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH -v2] IRQ, cpu-hotplug: Fix a race between CPU hotplug and IRQ desc alloc/free
Date
Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, 5 Sep 2017, Huang, Ying wrote:
>
>> From: Huang Ying <ying.huang@intel.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.

The below patch fixed the issue on my test system,

Tested-by: "Huang, Ying" <ying.huang@intel.com>

Best Regards,
Huang, Ying

> 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);

\
 
 \ /
  Last update: 2017-09-05 13:00    [W:0.075 / U:18.924 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site