| Message ID | 1470348549-10855-1-git-send-email-keith.busch@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Aug 4, 2016 at 4:09 PM, Keith Busch <keith.busch@intel.com> wrote: > We can't initialize the list head on deletion as this causes the node > to point to itself, looping infinitely if the vmd IRQ handler happens > to be servicing that node. > > The list initialization supposed to fix a bug from multiple calls to > disable the same IRQ. We can fix this instead just checking if the > previous pointer indicates it was already deleted. > Should this have one or possibly more"fixes" lines here? ???Fixes: 97e9230 ("x86/PCI: VMD: Initialize list item in IRQ disable")??? > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > arch/x86/pci/vmd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c > index e88b417..2294907 100644 > --- a/arch/x86/pci/vmd.c > +++ b/arch/x86/pci/vmd.c > @@ -136,8 +136,8 @@ static void vmd_irq_disable(struct irq_data *data) > data->chip->irq_mask(data); > > raw_spin_lock_irqsave(&list_lock, flags); > - list_del_rcu(&vmdirq->node); > - INIT_LIST_HEAD_RCU(&vmdirq->node); > + if (vmdirq->node.prev != LIST_POISON2) > + list_del_rcu(&vmdirq->node); > raw_spin_unlock_irqrestore(&list_lock, flags); > } > > -- > 2.7.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes that's the one it fixes -----Original Message----- From: Myron Stowe [mailto:myron.stowe@gmail.com] Sent: Thursday, August 4, 2016 4:48 PM To: Busch, Keith <keith.busch@intel.com> Cc: linux-pci <linux-pci@vger.kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; Derrick, Jonathan <jonathan.derrick@intel.com> Subject: Re: [PATCH 1/2] vmd: Fix infinite loop executing irq's On Thu, Aug 4, 2016 at 4:09 PM, Keith Busch <keith.busch@intel.com> wrote: > We can't initialize the list head on deletion as this causes the node > to point to itself, looping infinitely if the vmd IRQ handler happens > to be servicing that node. > > The list initialization supposed to fix a bug from multiple calls to > disable the same IRQ. We can fix this instead just checking if the > previous pointer indicates it was already deleted. > Should this have one or possibly more"fixes" lines here? ???Fixes: 97e9230 ("x86/PCI: VMD: Initialize list item in IRQ disable")??? > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > arch/x86/pci/vmd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c index > e88b417..2294907 100644 > --- a/arch/x86/pci/vmd.c > +++ b/arch/x86/pci/vmd.c > @@ -136,8 +136,8 @@ static void vmd_irq_disable(struct irq_data *data) > data->chip->irq_mask(data); > > raw_spin_lock_irqsave(&list_lock, flags); > - list_del_rcu(&vmdirq->node); > - INIT_LIST_HEAD_RCU(&vmdirq->node); > + if (vmdirq->node.prev != LIST_POISON2) > + list_del_rcu(&vmdirq->node); > raw_spin_unlock_irqrestore(&list_lock, flags); } > > -- > 2.7.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" > in the body of a message to majordomo@vger.kernel.org More majordomo > info at http://vger.kernel.org/majordomo-info.html
Hi Keith, On Thu, Aug 04, 2016 at 04:09:08PM -0600, Keith Busch wrote: > We can't initialize the list head on deletion as this causes the node > to point to itself, looping infinitely if the vmd IRQ handler happens > to be servicing that node. > > The list initialization supposed to fix a bug from multiple calls to > disable the same IRQ. We can fix this instead just checking if the > previous pointer indicates it was already deleted. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > arch/x86/pci/vmd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c > index e88b417..2294907 100644 > --- a/arch/x86/pci/vmd.c > +++ b/arch/x86/pci/vmd.c > @@ -136,8 +136,8 @@ static void vmd_irq_disable(struct irq_data *data) > data->chip->irq_mask(data); > > raw_spin_lock_irqsave(&list_lock, flags); > - list_del_rcu(&vmdirq->node); > - INIT_LIST_HEAD_RCU(&vmdirq->node); > + if (vmdirq->node.prev != LIST_POISON2) > + list_del_rcu(&vmdirq->node); It doesn't seem quite right to test for LIST_POISON2. It seems like a little too much knowledge of list internals. There are no other similar tests in the kernel. Surely this isn't the only case where we need to remove from a list that another thread might be traversing. I would look for other similar situations and copy the way they handle it. I think I saw a Fixes: tag later, so I assume you'll pick that up for v2. Should this also be tagged for stable? Are there any bug reports we should reference? > raw_spin_unlock_irqrestore(&list_lock, flags); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 05, 2016 at 12:03:02PM -0500, Bjorn Helgaas wrote: > It doesn't seem quite right to test for LIST_POISON2. It seems like a > little too much knowledge of list internals. There are no other > similar tests in the kernel. Surely this isn't the only case where we > need to remove from a list that another thread might be traversing. I > would look for other similar situations and copy the way they handle > it. Yeah, I agree that's abusing internal knowledge of the API. We can track this state another way. Will fix and resend. > I think I saw a Fixes: tag later, so I assume you'll pick that up for > v2. Should this also be tagged for stable? Are there any bug reports > we should reference? I'll add the fixes in the v2. This bug was reported only on internal Intel bug tracking, but I can still add a "Reported-by" and "Tested-by" tags. And no need for a stable since the bug was introduced in 4.8 merge window. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c index e88b417..2294907 100644 --- a/arch/x86/pci/vmd.c +++ b/arch/x86/pci/vmd.c @@ -136,8 +136,8 @@ static void vmd_irq_disable(struct irq_data *data) data->chip->irq_mask(data); raw_spin_lock_irqsave(&list_lock, flags); - list_del_rcu(&vmdirq->node); - INIT_LIST_HEAD_RCU(&vmdirq->node); + if (vmdirq->node.prev != LIST_POISON2) + list_del_rcu(&vmdirq->node); raw_spin_unlock_irqrestore(&list_lock, flags); }
We can't initialize the list head on deletion as this causes the node to point to itself, looping infinitely if the vmd IRQ handler happens to be servicing that node. The list initialization supposed to fix a bug from multiple calls to disable the same IRQ. We can fix this instead just checking if the previous pointer indicates it was already deleted. Signed-off-by: Keith Busch <keith.busch@intel.com> --- arch/x86/pci/vmd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)