[PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

[PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

While moving our XenoLinux patches to 4.2-rc I noticed bogus "already
mapped" messages resulting from Linux (legitimately) writing RTEs with
only the mask bit set. Clearly we shouldn't even attempt to create a
pIRQ <-> IRQ mapping from such RTEs.

In the course of this I also found that the respective message isn't
really useful without also printing the pre-existing mapping. And I
noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
never allowing and domain to control that interrupt.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2371,9 +2371,14 @@ int ioapic_guest_write(unsigned long phy
      * pirq and irq mapping. Where the GSI is greater than 256, we assume
      * that dom0 pirq == irq.
      */
-    pirq = (irq >= 256) ? irq : rte.vector;
-    if ( (pirq < 0) || (pirq >= hardware_domain->nr_pirqs) )
-        return -EINVAL;
+    if ( !rte.mask )
+    {
+        pirq = (irq >= 256) ? irq : rte.vector;
+        if ( pirq >= hardware_domain->nr_pirqs )
+            return -EINVAL;
+    }
+    else
+        pirq = -1;
     
     if ( desc->action )
     {
@@ -2408,12 +2413,15 @@ int ioapic_guest_write(unsigned long phy
 
         printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
     }
-    spin_lock(&hardware_domain->event_lock);
-    ret = map_domain_pirq(hardware_domain, pirq, irq,
-            MAP_PIRQ_TYPE_GSI, NULL);
-    spin_unlock(&hardware_domain->event_lock);
-    if ( ret < 0 )
-        return ret;
+    if ( pirq >= 0 )
+    {
+        spin_lock(&hardware_domain->event_lock);
+        ret = map_domain_pirq(hardware_domain, pirq, irq,
+                              MAP_PIRQ_TYPE_GSI, NULL);
+        spin_unlock(&hardware_domain->event_lock);
+        if ( ret < 0 )
+            return ret;
+    }
 
     spin_lock_irqsave(&ioapic_lock, flags);
     /* Set the correct irq-handling type. */
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1906,7 +1906,7 @@ int map_domain_pirq(
     if ( !irq_access_permitted(current->domain, irq))
         return -EPERM;
 
-    if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs )
+    if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs )
     {
         dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n",
                 d->domain_id, pirq, irq);
@@ -1919,8 +1919,9 @@ int map_domain_pirq(
     if ( (old_irq > 0 && (old_irq != irq) ) ||
          (old_pirq && (old_pirq != pirq)) )
     {
-        dprintk(XENLOG_G_WARNING, "dom%d: pirq %d or irq %d already mapped\n",
-                d->domain_id, pirq, irq);
+        dprintk(XENLOG_G_WARNING,
+                "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
+                d->domain_id, pirq, irq, old_pirq, old_irq);
         return 0;
     }
 




[-- Attachment #2: x86-IO-APIC-IRQ-mapping.patch --]
[-- Type: text/plain, Size: 2971 bytes --]

x86/IO-APIC: don't create pIRQ mapping from masked RTE

While moving our XenoLinux patches to 4.2-rc I noticed bogus "already
mapped" messages resulting from Linux (legitimately) writing RTEs with
only the mask bit set. Clearly we shouldn't even attempt to create a
pIRQ <-> IRQ mapping from such RTEs.

In the course of this I also found that the respective message isn't
really useful without also printing the pre-existing mapping. And I
noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
never allowing and domain to control that interrupt.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2371,9 +2371,14 @@ int ioapic_guest_write(unsigned long phy
      * pirq and irq mapping. Where the GSI is greater than 256, we assume
      * that dom0 pirq == irq.
      */
-    pirq = (irq >= 256) ? irq : rte.vector;
-    if ( (pirq < 0) || (pirq >= hardware_domain->nr_pirqs) )
-        return -EINVAL;
+    if ( !rte.mask )
+    {
+        pirq = (irq >= 256) ? irq : rte.vector;
+        if ( pirq >= hardware_domain->nr_pirqs )
+            return -EINVAL;
+    }
+    else
+        pirq = -1;
     
     if ( desc->action )
     {
@@ -2408,12 +2413,15 @@ int ioapic_guest_write(unsigned long phy
 
         printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
     }
-    spin_lock(&hardware_domain->event_lock);
-    ret = map_domain_pirq(hardware_domain, pirq, irq,
-            MAP_PIRQ_TYPE_GSI, NULL);
-    spin_unlock(&hardware_domain->event_lock);
-    if ( ret < 0 )
-        return ret;
+    if ( pirq >= 0 )
+    {
+        spin_lock(&hardware_domain->event_lock);
+        ret = map_domain_pirq(hardware_domain, pirq, irq,
+                              MAP_PIRQ_TYPE_GSI, NULL);
+        spin_unlock(&hardware_domain->event_lock);
+        if ( ret < 0 )
+            return ret;
+    }
 
     spin_lock_irqsave(&ioapic_lock, flags);
     /* Set the correct irq-handling type. */
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1906,7 +1906,7 @@ int map_domain_pirq(
     if ( !irq_access_permitted(current->domain, irq))
         return -EPERM;
 
-    if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs )
+    if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs )
     {
         dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n",
                 d->domain_id, pirq, irq);
@@ -1919,8 +1919,9 @@ int map_domain_pirq(
     if ( (old_irq > 0 && (old_irq != irq) ) ||
          (old_pirq && (old_pirq != pirq)) )
     {
-        dprintk(XENLOG_G_WARNING, "dom%d: pirq %d or irq %d already mapped\n",
-                d->domain_id, pirq, irq);
+        dprintk(XENLOG_G_WARNING,
+                "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
+                d->domain_id, pirq, irq, old_pirq, old_irq);
         return 0;
     }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

[Andrew Cooper]

On 21/08/15 09:41, Jan Beulich wrote:
> While moving our XenoLinux patches to 4.2-rc I noticed bogus "already
> mapped" messages resulting from Linux (legitimately) writing RTEs with
> only the mask bit set. Clearly we shouldn't even attempt to create a
> pIRQ <-> IRQ mapping from such RTEs.

Oops.  Agreed.

>
> In the course of this I also found that the respective message isn't
> really useful without also printing the pre-existing mapping. And I
> noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
> never allowing and domain to control that interrupt.

s/and/a/ ? (I can't quite parse the original statement)

Also, doesn't irq_access_permitted() catch the irq0 case?

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> with one suggestion

>
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2371,9 +2371,14 @@ int ioapic_guest_write(unsigned long phy
>       * pirq and irq mapping. Where the GSI is greater than 256, we assume
>       * that dom0 pirq == irq.
>       */
> -    pirq = (irq >= 256) ? irq : rte.vector;
> -    if ( (pirq < 0) || (pirq >= hardware_domain->nr_pirqs) )
> -        return -EINVAL;
> +    if ( !rte.mask )
> +    {
> +        pirq = (irq >= 256) ? irq : rte.vector;
> +        if ( pirq >= hardware_domain->nr_pirqs )
> +            return -EINVAL;
> +    }
> +    else
> +        pirq = -1;
>      
>      if ( desc->action )
>      {
> @@ -2408,12 +2413,15 @@ int ioapic_guest_write(unsigned long phy
>  
>          printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
>      }
> -    spin_lock(&hardware_domain->event_lock);
> -    ret = map_domain_pirq(hardware_domain, pirq, irq,
> -            MAP_PIRQ_TYPE_GSI, NULL);
> -    spin_unlock(&hardware_domain->event_lock);
> -    if ( ret < 0 )
> -        return ret;
> +    if ( pirq >= 0 )
> +    {
> +        spin_lock(&hardware_domain->event_lock);
> +        ret = map_domain_pirq(hardware_domain, pirq, irq,
> +                              MAP_PIRQ_TYPE_GSI, NULL);
> +        spin_unlock(&hardware_domain->event_lock);
> +        if ( ret < 0 )
> +            return ret;
> +    }
>  
>      spin_lock_irqsave(&ioapic_lock, flags);
>      /* Set the correct irq-handling type. */
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1906,7 +1906,7 @@ int map_domain_pirq(
>      if ( !irq_access_permitted(current->domain, irq))
>          return -EPERM;
>  

I would be tempted to put a comment here stating that irq0 is definitely
a xen-reserved irq.  Otherwise, it is odd to have the mismatch between
pirq and irq.

~Andrew

> -    if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs )
> +    if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs )
>      {
>          dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n",
>                  d->domain_id, pirq, irq);
> @@ -1919,8 +1919,9 @@ int map_domain_pirq(
>      if ( (old_irq > 0 && (old_irq != irq) ) ||
>           (old_pirq && (old_pirq != pirq)) )
>      {
> -        dprintk(XENLOG_G_WARNING, "dom%d: pirq %d or irq %d already mapped\n",
> -                d->domain_id, pirq, irq);
> +        dprintk(XENLOG_G_WARNING,
> +                "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
> +                d->domain_id, pirq, irq, old_pirq, old_irq);
>          return 0;
>      }
>  
>
>
>

Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

[Jan Beulich]

>>> On 21.08.15 at 16:58, <andrew.cooper3@citrix.com> wrote:
> On 21/08/15 09:41, Jan Beulich wrote:
>> In the course of this I also found that the respective message isn't
>> really useful without also printing the pre-existing mapping. And I
>> noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
>> never allowing and domain to control that interrupt.
> 
> s/and/a/ ? (I can't quite parse the original statement)

Ouch - "a" was meant.

> Also, doesn't irq_access_permitted() catch the irq0 case?

It should, yes, but ...

>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -1906,7 +1906,7 @@ int map_domain_pirq(
>>      if ( !irq_access_permitted(current->domain, irq))
>>          return -EPERM;
> 
> I would be tempted to put a comment here stating that irq0 is definitely
> a xen-reserved irq.  Otherwise, it is odd to have the mismatch between
> pirq and irq.

Such a "mismatch" was already there (albeit I wouldn't call it a
mismatch, since from an abstract pov the two number spaces are
entirely distinct), specifically ...

>> -    if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs )
>> +    if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs )
>>      {
>>          dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n",
>>                  d->domain_id, pirq, irq);
>> @@ -1919,8 +1919,9 @@ int map_domain_pirq(
>>      if ( (old_irq > 0 && (old_irq != irq) ) ||
>>           (old_pirq && (old_pirq != pirq)) )

... here.

Jan

>>      {
>> -        dprintk(XENLOG_G_WARNING, "dom%d: pirq %d or irq %d already mapped\n",
>> -                d->domain_id, pirq, irq);
>> +        dprintk(XENLOG_G_WARNING,
>> +                "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
>> +                d->domain_id, pirq, irq, old_pirq, old_irq);
>>          return 0;
>>      }
>>  

Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

[Andrew Cooper]

On 21/08/15 16:35, Jan Beulich wrote:
>>>> On 21.08.15 at 16:58, <andrew.cooper3@citrix.com> wrote:
>> On 21/08/15 09:41, Jan Beulich wrote:
>>> In the course of this I also found that the respective message isn't
>>> really useful without also printing the pre-existing mapping. And I
>>> noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
>>> never allowing and domain to control that interrupt.
>> s/and/a/ ? (I can't quite parse the original statement)
> Ouch - "a" was meant.
>
>> Also, doesn't irq_access_permitted() catch the irq0 case?
> It should, yes, but ...
>
>>> --- a/xen/arch/x86/irq.c
>>> +++ b/xen/arch/x86/irq.c
>>> @@ -1906,7 +1906,7 @@ int map_domain_pirq(
>>>      if ( !irq_access_permitted(current->domain, irq))
>>>          return -EPERM;
>> I would be tempted to put a comment here stating that irq0 is definitely
>> a xen-reserved irq.  Otherwise, it is odd to have the mismatch between
>> pirq and irq.
> Such a "mismatch" was already there (albeit I wouldn't call it a
> mismatch, since from an abstract pov the two number spaces are
> entirely distinct), specifically ...
>
>>> -    if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs )
>>> +    if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs )
>>>      {
>>>          dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n",
>>>                  d->domain_id, pirq, irq);
>>> @@ -1919,8 +1919,9 @@ int map_domain_pirq(
>>>      if ( (old_irq > 0 && (old_irq != irq) ) ||
>>>           (old_pirq && (old_pirq != pirq)) )
> ... here.

Right, but irq0 is a valid irq which makes it suspicious to drop it at
the validity check.

I am not fussed too much with bikeshedding the issue.

~Andrew

Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

[Wei Liu]

On Fri, Aug 21, 2015 at 02:41:32AM -0600, Jan Beulich wrote:
> While moving our XenoLinux patches to 4.2-rc I noticed bogus "already
> mapped" messages resulting from Linux (legitimately) writing RTEs with
> only the mask bit set. Clearly we shouldn't even attempt to create a
> pIRQ <-> IRQ mapping from such RTEs.
> 
> In the course of this I also found that the respective message isn't
> really useful without also printing the pre-existing mapping. And I
> noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
> never allowing and domain to control that interrupt.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Wei Liu <wei.liu2@citrix.com>