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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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) ?

~Andrew

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