Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933887AbbLWOLM (ORCPT ); Wed, 23 Dec 2015 09:11:12 -0500 Received: from mga01.intel.com ([192.55.52.88]:48313 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757192AbbLWOKb (ORCPT ); Wed, 23 Dec 2015 09:10:31 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,469,1444719600"; d="scan'208";a="622778269" From: Jiang Liu To: Thomas Gleixner , Joe Lawrence , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Jiang Liu Cc: Jeremiah Mahler , Borislav Petkov , andy.shevchenko@gmail.com, Guenter Roeck , linux-kernel@vger.kernel.org Subject: [Bugfix v2 4/5] x86/irq: Fix a race condition between vector assigning and cleanup Date: Wed, 23 Dec 2015 22:13:29 +0800 Message-Id: <1450880014-11741-4-git-send-email-jiang.liu@linux.intel.com> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1450880014-11741-1-git-send-email-jiang.liu@linux.intel.com> References: <1450880014-11741-1-git-send-email-jiang.liu@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6218 Lines: 184 Joe Lawrence reported an use after release issue related to x86 IRQ management code. Please refer to following link for more information: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1026840.html Thomas pointed out that it's caused by a race condition between __assign_irq_vector() and __send_cleanup_vector(). Based on Thomas' draft patch, we solve this race condition by: 1) Use move_in_progress to signal that an IRQ cleanup IPI is needed 2) Use old_domain to save old CPU mask for IRQ cleanup 3) Use vector to protect move_in_progress and old_domain This bugfix patch also helps to get rid of that atomic allocation in __send_cleanup_vector(). Signed-off-by: Jiang Liu --- arch/x86/kernel/apic/vector.c | 76 ++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 42 deletions(-) diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c index f648fce39d5e..ab54b296a7d0 100644 --- a/arch/x86/kernel/apic/vector.c +++ b/arch/x86/kernel/apic/vector.c @@ -119,7 +119,7 @@ static int __assign_irq_vector(int irq, struct apic_chip_data *d, int cpu, err; unsigned int dest; - if (d->move_in_progress) + if (cpumask_intersects(d->old_domain, cpu_online_mask)) return -EBUSY; /* Only try and allocate irqs on cpus that are present */ @@ -141,13 +141,14 @@ static int __assign_irq_vector(int irq, struct apic_chip_data *d, cpumask_and(used_cpumask, d->domain, vector_cpumask); err = apic->cpu_mask_to_apicid_and(mask, used_cpumask, &dest); - if (err || cpumask_equal(vector_cpumask, d->domain)) + if (err) break; - cpumask_andnot(d->old_domain, d->domain, - vector_cpumask); - d->move_in_progress = - cpumask_intersects(d->old_domain, cpu_online_mask); - cpumask_copy(d->domain, used_cpumask); + d->cfg.dest_apicid = dest; + if (!cpumask_equal(vector_cpumask, d->domain)) { + cpumask_andnot(d->old_domain, d->domain, + vector_cpumask); + cpumask_copy(d->domain, used_cpumask); + } break; } @@ -180,22 +181,20 @@ next: /* Found one! */ current_vector = vector; current_offset = offset; - if (d->cfg.vector) { + if (d->cfg.vector) cpumask_copy(d->old_domain, d->domain); - d->move_in_progress = - cpumask_intersects(d->old_domain, cpu_online_mask); - } + d->cfg.vector = vector; + d->cfg.dest_apicid = dest; for_each_cpu_and(new_cpu, vector_cpumask, cpu_online_mask) per_cpu(vector_irq, new_cpu)[vector] = irq_to_desc(irq); - d->cfg.vector = vector; cpumask_copy(d->domain, vector_cpumask); err = 0; break; } if (!err) { - /* cache destination APIC IDs into cfg->dest_apicid */ - d->cfg.dest_apicid = dest; + cpumask_and(d->old_domain, d->old_domain, cpu_online_mask); + d->move_in_progress = !cpumask_empty(d->old_domain); } return err; @@ -227,7 +226,7 @@ static int assign_irq_vector_policy(int irq, int node, 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; - - 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); + if (cpumask_test_cpu(cpu, data->domain)) { + vector = data->cfg.vector; + per_cpu(vector_irq, cpu)[vector] = desc; + } + } } /* Mark the free vectors */ for (vector = 0; vector < NR_VECTORS; ++vector) { @@ -506,20 +504,17 @@ static struct irq_chip lapic_controller = { #ifdef CONFIG_SMP static void __send_cleanup_vector(struct apic_chip_data *data) { - cpumask_var_t cleanup_mask; - - if (unlikely(!alloc_cpumask_var(&cleanup_mask, GFP_ATOMIC))) { - unsigned int i; + unsigned long flags; - for_each_cpu_and(i, data->old_domain, cpu_online_mask) - apic->send_IPI_mask(cpumask_of(i), - IRQ_MOVE_CLEANUP_VECTOR); - } else { - cpumask_and(cleanup_mask, data->old_domain, cpu_online_mask); - apic->send_IPI_mask(cleanup_mask, IRQ_MOVE_CLEANUP_VECTOR); - free_cpumask_var(cleanup_mask); - } + raw_spin_lock_irqsave(&vector_lock, flags); + if (!data->move_in_progress) + goto out_unlock; data->move_in_progress = 0; + cpumask_and(data->old_domain, data->old_domain, cpu_online_mask); + if (!cpumask_empty(data->old_domain)) + apic->send_IPI_mask(data->old_domain, IRQ_MOVE_CLEANUP_VECTOR); +out_unlock: + raw_spin_unlock_irqrestore(&vector_lock, flags); } void send_cleanup_vector(struct irq_cfg *cfg) @@ -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; - - if (vector == data->cfg.vector && - cpumask_test_cpu(me, data->domain)) + if (!cpumask_test_cpu(me, data->old_domain)) goto unlock; irr = apic_read(APIC_IRR + (vector / 32 * 0x10)); @@ -586,6 +577,7 @@ asmlinkage __visible void smp_irq_move_cleanup_interrupt(void) goto unlock; } __this_cpu_write(vector_irq[vector], VECTOR_UNUSED); + cpumask_clear_cpu(me, data->old_domain); unlock: raw_spin_unlock(&desc->lock); } -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/