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

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:	Wed, 30 Dec 2015 18:25:35 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Jiang Liu <jiang.liu@...ux.intel.com>
cc:	Joe Lawrence <joe.lawrence@...atus.com>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	Jeremiah Mahler <jmmahler@...il.com>,
	Borislav Petkov <bp@...en8.de>, andy.shevchenko@...il.com,
	Guenter Roeck <linux@...ck-us.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [Bugfix v2 4/5] x86/irq: Fix a race condition between vector
 assigning and cleanup

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/