Re: [Xen-devel] [PATCH 1/3] x86/IRQ: conditionally preserve access permission on map error paths

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.12.17 at 17:07, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/12/17 10:32, Jan Beulich wrote:
>> Permissions that had been granted before should not be revoked when
>> handling unrelated errors.
>>
>> Reported-by: HW42 <hw42@xxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -1918,6 +1918,7 @@ int map_domain_pirq(
>>      struct irq_desc *desc;
>>      unsigned long flags;
>>      DECLARE_BITMAP(prepared, MAX_MSI_IRQS) = {};
>> +    DECLARE_BITMAP(granted, MAX_MSI_IRQS) = {};
>>  
>>      ASSERT(spin_is_locked(&d->event_lock));
>>  
>> @@ -1951,13 +1952,17 @@ int map_domain_pirq(
>>          return ret;
>>      }
>>  
>> -    ret = irq_permit_access(d, irq);
>> -    if ( ret )
>> +    if ( likely(!irq_access_permitted(d, irq)) )
>>      {
>> -        printk(XENLOG_G_ERR
>> -               "dom%d: could not permit access to IRQ%d (pirq %d)\n",
>> -               d->domain_id, irq, pirq);
>> -        return ret;
>> +        ret = irq_permit_access(d, irq);
>> +        if ( ret )
>> +        {
>> +            printk(XENLOG_G_ERR
>> +                   "dom%d: could not permit access to IRQ%d (pirq %d)\n",
>> +                  d->domain_id, irq, pirq);
>> +            return ret;
>> +        }
>> +        __set_bit(0, granted);
>>      }
>>  
>>      ret = prepare_domain_irq_pirq(d, irq, pirq, &info);
>> @@ -2042,10 +2047,15 @@ int map_domain_pirq(
>>                  __set_bit(nr, prepared);
>>              msi_desc[nr].irq = irq;
>>  
>> -            if ( irq_permit_access(d, irq) != 0 )
>> -                printk(XENLOG_G_WARNING
>> -                       "dom%d: could not permit access to IRQ%d (pirq 
>> %d)\n",
>> -                       d->domain_id, irq, pirq);
>> +            if ( likely(!irq_access_permitted(d, irq)) )
>> +            {
>> +                if ( irq_permit_access(d, irq) )
>> +                    printk(XENLOG_G_WARNING
>> +                           "dom%d: could not permit access to IRQ%d (pirq 
>> %d)\n",
>> +                           d->domain_id, irq, pirq);
>> +                else
>> +                    __set_bit(0, granted);
>> +            }
>>  
>>              desc = irq_to_desc(irq);
>>              spin_lock_irqsave(&desc->lock, flags);
>> @@ -2074,7 +2084,8 @@ int map_domain_pirq(
>>              }
>>              while ( nr )
>>              {
>> -                if ( irq >= 0 && irq_deny_access(d, irq) )
>> +                if ( irq >= 0 && test_bit(nr, granted) &&
> 
> You only ever set bit 0 of granted, but you test each of them here. 
> Something seems wrong.
> 
> Should the previous hunk be __set_bit(nr, granted) ?

Oh, yes of course, good catch. I'm sure it was right in an initial
version of the patch, but must have got broken in a re-work /
re-base.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel