Re: [PATCH -v2] IRQ, cpu-hotplug: Fix a race between CPU hotplug and IRQ desc alloc/free

[Huang\, Ying]

Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:

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

Sorry about that.

> 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.

Yes.  This is much better than my description.

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

I will test this patch

Best Regards,
Huang, Ying