From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751886AbdIEIy2 (ORCPT ); Tue, 5 Sep 2017 04:54:28 -0400 Received: from mga11.intel.com ([192.55.52.93]:19127 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751524AbdIEIyU (ORCPT ); Tue, 5 Sep 2017 04:54:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,479,1498546800"; d="scan'208";a="125643259" From: "Huang\, Ying" To: Thomas Gleixner Cc: "Huang\, Ying" , Subject: Re: [PATCH -v2] IRQ, cpu-hotplug: Fix a race between CPU hotplug and IRQ desc alloc/free References: <20170905021758.8530-1-ying.huang@intel.com> Date: Tue, 05 Sep 2017 16:54:15 +0800 In-Reply-To: (Thomas Gleixner's message of "Tue, 5 Sep 2017 10:12:20 +0200") Message-ID: <871snlwmo8.fsf@yhuang-mobile.sh.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thomas Gleixner writes: > On Tue, 5 Sep 2017, Huang, Ying wrote: > >> From: Huang Ying >> >> 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