Re: [Bugfix v2 4/5] x86/irq: Fix a race condition between vector assigning and cleanup

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 23 Dec 2015, Jiang Liu wrote:
>  static void clear_irq_vector(int irq, struct apic_chip_data *data)
>  {
> -	struct irq_desc *desc;
> +	struct irq_desc *desc = irq_to_desc(irq);
>  	int cpu, vector = data->cfg.vector;
>  
>  	BUG_ON(!vector);
> @@ -236,10 +235,6 @@ static void clear_irq_vector(int irq, struct apic_chip_data *data)
>  	data->cfg.vector = 0;
>  	cpumask_clear(data->domain);
>  
> -	if (likely(!data->move_in_progress))
> -		return;

Why are you removing this?

> -
> -	desc = irq_to_desc(irq);
>  	for_each_cpu_and(cpu, data->old_domain, cpu_online_mask) {
>  		for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS;
>  		     vector++) {
> @@ -421,10 +416,13 @@ static void __setup_vector_irq(int cpu)
>  		struct irq_data *idata = irq_desc_get_irq_data(desc);
>  
>  		data = apic_chip_data(idata);
> -		if (!data || !cpumask_test_cpu(cpu, data->domain))
> -			continue;
> -		vector = data->cfg.vector;
> -		per_cpu(vector_irq, cpu)[vector] = desc;
> +		if (data) {
> +			cpumask_clear_cpu(cpu, data->old_domain);

Why would the newly online cpu be in data->old_domain?

> +			if (cpumask_test_cpu(cpu, data->domain)) {
> +				vector = data->cfg.vector;
> +				per_cpu(vector_irq, cpu)[vector] = desc;
> +			}
> +		}
> @@ -563,14 +558,10 @@ asmlinkage __visible void smp_irq_move_cleanup_interrupt(void)
>  			goto unlock;
>  
>  		/*
> -		 * Check if the irq migration is in progress. If so, we
> -		 * haven't received the cleanup request yet for this irq.
> +		 * Nothing to cleanup if this cpu is not set
> +		 * in the old_domain mask.
>  		 */
> -		if (data->move_in_progress)
> -			goto unlock;

Removing this is broken. If data->move_in_progress is set, then you cannot
clear the vector. If there are two interrupt moves pending then the IPI of the
first one will clear the second one as well, which might be still targeted to
this cpu.

This whole patch set is way too complex. A lot of tiny changes here and there
and none of them is documented.

Thanks,

	tglx