[PATCH 1/2] vmd: Fix infinite loop executing irq's

[PATCH 1/2] vmd: Fix infinite loop executing irq's

[Keith Busch]

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);
 	raw_spin_unlock_irqrestore(&list_lock, flags);
 }
 
-- 
2.7.2

[PATCH 2/2] vmd: Synchronize with RCU freeing msi irq descs

[Keith Busch]

This patch fixes a potential race when disabling MSI/MSI-x on a VMD domain
device. If the vmd interrupt service is running, it may see a disabled
irq. We can synchronize rcu just before freeing the msi descriptor. This
is safe since the irq_desc lock isn't held, and the descriptor is valid
even though it is disabled. After vmd_msi_free, though, the handler is
reinitialiazed to handle_bad_irq, so we can't let the vmd isr's list
iteration see the disabled irq after this.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 arch/x86/pci/vmd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
index 2294907..e3c9b9e 100644
--- a/arch/x86/pci/vmd.c
+++ b/arch/x86/pci/vmd.c
@@ -213,6 +213,8 @@ static void vmd_msi_free(struct irq_domain *domain,
 	struct vmd_irq *vmdirq = irq_get_chip_data(virq);
 	unsigned long flags;
 
+	synchronize_rcu();
+
 	/* XXX: Potential optimization to rebalance */
 	raw_spin_lock_irqsave(&list_lock, flags);
 	vmdirq->irq->count--;
-- 
2.7.2

Re: [PATCH 1/2] vmd: Fix infinite loop executing irq's

[Myron Stowe]

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

RE: [PATCH 1/2] vmd: Fix infinite loop executing irq's

[Derrick, Jonathan]

WWVzIHRoYXQncyB0aGUgb25lIGl0IGZpeGVzDQoNCi0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0t
DQpGcm9tOiBNeXJvbiBTdG93ZSBbbWFpbHRvOm15cm9uLnN0b3dlQGdtYWlsLmNvbV0gDQpTZW50
OiBUaHVyc2RheSwgQXVndXN0IDQsIDIwMTYgNDo0OCBQTQ0KVG86IEJ1c2NoLCBLZWl0aCA8a2Vp
dGguYnVzY2hAaW50ZWwuY29tPg0KQ2M6IGxpbnV4LXBjaSA8bGludXgtcGNpQHZnZXIua2VybmVs
Lm9yZz47IEJqb3JuIEhlbGdhYXMgPGJoZWxnYWFzQGdvb2dsZS5jb20+OyBEZXJyaWNrLCBKb25h
dGhhbiA8am9uYXRoYW4uZGVycmlja0BpbnRlbC5jb20+DQpTdWJqZWN0OiBSZTogW1BBVENIIDEv
Ml0gdm1kOiBGaXggaW5maW5pdGUgbG9vcCBleGVjdXRpbmcgaXJxJ3MNCg0KT24gVGh1LCBBdWcg
NCwgMjAxNiBhdCA0OjA5IFBNLCBLZWl0aCBCdXNjaCA8a2VpdGguYnVzY2hAaW50ZWwuY29tPiB3
cm90ZToNCj4gV2UgY2FuJ3QgaW5pdGlhbGl6ZSB0aGUgbGlzdCBoZWFkIG9uIGRlbGV0aW9uIGFz
IHRoaXMgY2F1c2VzIHRoZSBub2RlIA0KPiB0byBwb2ludCB0byBpdHNlbGYsIGxvb3BpbmcgaW5m
aW5pdGVseSBpZiB0aGUgdm1kIElSUSBoYW5kbGVyIGhhcHBlbnMgDQo+IHRvIGJlIHNlcnZpY2lu
ZyB0aGF0IG5vZGUuDQo+DQo+IFRoZSBsaXN0IGluaXRpYWxpemF0aW9uIHN1cHBvc2VkIHRvIGZp
eCBhIGJ1ZyBmcm9tIG11bHRpcGxlIGNhbGxzIHRvIA0KPiBkaXNhYmxlIHRoZSBzYW1lIElSUS4g
V2UgY2FuIGZpeCB0aGlzIGluc3RlYWQganVzdCBjaGVja2luZyBpZiB0aGUgDQo+IHByZXZpb3Vz
IHBvaW50ZXIgaW5kaWNhdGVzIGl0IHdhcyBhbHJlYWR5IGRlbGV0ZWQuDQo+DQoNClNob3VsZCB0
aGlzIGhhdmUgb25lIG9yIHBvc3NpYmx5IG1vcmUiZml4ZXMiIGxpbmVzIGhlcmU/DQoNCj8/P0Zp
eGVzOiA5N2U5MjMwICgieDg2L1BDSTogVk1EOiBJbml0aWFsaXplIGxpc3QgaXRlbSBpbiBJUlEg
ZGlzYWJsZSIpPz8/DQoNCj4gU2lnbmVkLW9mZi1ieTogS2VpdGggQnVzY2ggPGtlaXRoLmJ1c2No
QGludGVsLmNvbT4NCj4gLS0tDQo+ICBhcmNoL3g4Ni9wY2kvdm1kLmMgfCA0ICsrLS0NCj4gIDEg
ZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+DQo+IGRpZmYg
LS1naXQgYS9hcmNoL3g4Ni9wY2kvdm1kLmMgYi9hcmNoL3g4Ni9wY2kvdm1kLmMgaW5kZXggDQo+
IGU4OGI0MTcuLjIyOTQ5MDcgMTAwNjQ0DQo+IC0tLSBhL2FyY2gveDg2L3BjaS92bWQuYw0KPiAr
KysgYi9hcmNoL3g4Ni9wY2kvdm1kLmMNCj4gQEAgLTEzNiw4ICsxMzYsOCBAQCBzdGF0aWMgdm9p
ZCB2bWRfaXJxX2Rpc2FibGUoc3RydWN0IGlycV9kYXRhICpkYXRhKQ0KPiAgICAgICAgIGRhdGEt
PmNoaXAtPmlycV9tYXNrKGRhdGEpOw0KPg0KPiAgICAgICAgIHJhd19zcGluX2xvY2tfaXJxc2F2
ZSgmbGlzdF9sb2NrLCBmbGFncyk7DQo+IC0gICAgICAgbGlzdF9kZWxfcmN1KCZ2bWRpcnEtPm5v
ZGUpOw0KPiAtICAgICAgIElOSVRfTElTVF9IRUFEX1JDVSgmdm1kaXJxLT5ub2RlKTsNCj4gKyAg
ICAgICBpZiAodm1kaXJxLT5ub2RlLnByZXYgIT0gTElTVF9QT0lTT04yKQ0KPiArICAgICAgICAg
ICAgICAgbGlzdF9kZWxfcmN1KCZ2bWRpcnEtPm5vZGUpOw0KPiAgICAgICAgIHJhd19zcGluX3Vu
bG9ja19pcnFyZXN0b3JlKCZsaXN0X2xvY2ssIGZsYWdzKTsgIH0NCj4NCj4gLS0NCj4gMi43LjIN
Cj4NCj4gLS0NCj4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUg
InVuc3Vic2NyaWJlIGxpbnV4LXBjaSIgDQo+IGluIHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBt
YWpvcmRvbW9Admdlci5rZXJuZWwub3JnIE1vcmUgbWFqb3Jkb21vIA0KPiBpbmZvIGF0ICBodHRw
Oi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwNCg==

Re: [PATCH 1/2] vmd: Fix infinite loop executing irq's

[Bjorn Helgaas]

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);
>  }
>  

Re: [PATCH 1/2] vmd: Fix infinite loop executing irq's

[Keith Busch]

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.

Re: [PATCH 2/2] vmd: Synchronize with RCU freeing msi irq descs

[Jon Derrick]

This one's fine

Acked-by Jon Derrick: <jonathan.derrick@intel.com>


On Thu, Aug 04, 2016 at 04:09:09PM -0600, Keith Busch wrote:
> This patch fixes a potential race when disabling MSI/MSI-x on a VMD domain
> device. If the vmd interrupt service is running, it may see a disabled
> irq. We can synchronize rcu just before freeing the msi descriptor. This
> is safe since the irq_desc lock isn't held, and the descriptor is valid
> even though it is disabled. After vmd_msi_free, though, the handler is
> reinitialiazed to handle_bad_irq, so we can't let the vmd isr's list
> iteration see the disabled irq after this.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  arch/x86/pci/vmd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
> index 2294907..e3c9b9e 100644
> --- a/arch/x86/pci/vmd.c
> +++ b/arch/x86/pci/vmd.c
> @@ -213,6 +213,8 @@ static void vmd_msi_free(struct irq_domain *domain,
>  	struct vmd_irq *vmdirq = irq_get_chip_data(virq);
>  	unsigned long flags;
>  
> +	synchronize_rcu();
> +
>  	/* XXX: Potential optimization to rebalance */
>  	raw_spin_lock_irqsave(&list_lock, flags);
>  	vmdirq->irq->count--;
> -- 
> 2.7.2
> 

Re: [PATCH 2/2] vmd: Synchronize with RCU freeing msi irq descs

[Jon Derrick]

Hi Bjorn,
Any chance of pulling this one in?

Thanks,
Jon

On Mon, Aug 08, 2016 at 10:47:21AM -0600, Jon Derrick wrote:
> This one's fine
> 
> Acked-by Jon Derrick: <jonathan.derrick@intel.com>
> 
> 
> On Thu, Aug 04, 2016 at 04:09:09PM -0600, Keith Busch wrote:
> > This patch fixes a potential race when disabling MSI/MSI-x on a VMD domain
> > device. If the vmd interrupt service is running, it may see a disabled
> > irq. We can synchronize rcu just before freeing the msi descriptor. This
> > is safe since the irq_desc lock isn't held, and the descriptor is valid
> > even though it is disabled. After vmd_msi_free, though, the handler is
> > reinitialiazed to handle_bad_irq, so we can't let the vmd isr's list
> > iteration see the disabled irq after this.
> > 
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
> > ---
> >  arch/x86/pci/vmd.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
> > index 2294907..e3c9b9e 100644
> > --- a/arch/x86/pci/vmd.c
> > +++ b/arch/x86/pci/vmd.c
> > @@ -213,6 +213,8 @@ static void vmd_msi_free(struct irq_domain *domain,
> >  	struct vmd_irq *vmdirq = irq_get_chip_data(virq);
> >  	unsigned long flags;
> >  
> > +	synchronize_rcu();
> > +
> >  	/* XXX: Potential optimization to rebalance */
> >  	raw_spin_lock_irqsave(&list_lock, flags);
> >  	vmdirq->irq->count--;
> > -- 
> > 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

Re: [PATCH 2/2] vmd: Synchronize with RCU freeing msi irq descs

[Bjorn Helgaas]

On Thu, Aug 04, 2016 at 04:09:09PM -0600, Keith Busch wrote:
> This patch fixes a potential race when disabling MSI/MSI-x on a VMD domain
> device. If the vmd interrupt service is running, it may see a disabled
> irq. We can synchronize rcu just before freeing the msi descriptor. This
> is safe since the irq_desc lock isn't held, and the descriptor is valid
> even though it is disabled. After vmd_msi_free, though, the handler is
> reinitialiazed to handle_bad_irq, so we can't let the vmd isr's list
> iteration see the disabled irq after this.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Applied with Jon's ack to pci/host-vmd for v4.9, thanks!

If anybody else reads this, this patch got lost for a while because it
was posted as the second patch of a series.  I had some comments on
the first patch that led to a little rework.  When that happens, I
mark the *entire* series as "Changes requested" or "Superseded"
because I'm expecting a complete repost with the rework.

It's too much work for me to track individual patches within a series.
So if you update one patch in a series, please just repost the entire
series, including other patches that may be unmodified.

Bjorn

> ---
>  arch/x86/pci/vmd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c
> index 2294907..e3c9b9e 100644
> --- a/arch/x86/pci/vmd.c
> +++ b/arch/x86/pci/vmd.c
> @@ -213,6 +213,8 @@ static void vmd_msi_free(struct irq_domain *domain,
>  	struct vmd_irq *vmdirq = irq_get_chip_data(virq);
>  	unsigned long flags;
>  
> +	synchronize_rcu();
> +
>  	/* XXX: Potential optimization to rebalance */
>  	raw_spin_lock_irqsave(&list_lock, flags);
>  	vmdirq->irq->count--;
> -- 
> 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