Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jul 18, 2019 at 11:29:39AM +0200, Roger Pau Monné wrote:
> On Wed, Jul 17, 2019 at 05:09:12PM +0200, Marek Marczykowski-Górecki wrote:
> > On Wed, Jul 17, 2019 at 11:54:35AM +0200, Roger Pau Monné wrote:
> > > On Wed, Jul 17, 2019 at 03:00:42AM +0200, Marek Marczykowski-Górecki 
> > > wrote:
> > > > @@ -220,14 +237,22 @@ void destroy_irq(unsigned int irq)
> > > >  
> > > >      BUG_ON(!MSI_IRQ(irq));
> > > >  
> > > > -    if ( hardware_domain )
> > > > +    if ( desc->creator_domid != DOMID_INVALID )
> > > >      {
> > > > -        int err = irq_deny_access(hardware_domain, irq);
> > > > +        struct domain *d = get_domain_by_id(desc->creator_domid);
> > > >  
> > > > -        if ( err )
> > > > -            printk(XENLOG_G_ERR
> > > > -                   "Could not revoke Dom0 access to IRQ%u (error 
> > > > %d)\n",
> > > > -                   irq, err);
> > > > +        if ( d && irq_access_permitted(d, irq) ) {
> > > > +            int err;
> > > > +
> > > > +            err = irq_deny_access(d, irq);
> > > > +            if ( err )
> > > > +                printk(XENLOG_G_ERR
> > > > +                       "Could not revoke Dom%u access to IRQ%u (error 
> > > > %d)\n",
> > > > +                       d->domain_id, irq, err);
> > > > +        }
> > > > +
> > > > +        if ( d )
> > > > +            put_domain(d);
> > > 
> > > Don't you need to set creator_domid = DOMID_INVALID in destroy_irq at
> > > some point?
> > > 
> > > Or else a failure in create_irq could leak the irq to it's previous
> > > owner. Note that init_one_irq_desc would only init the fields the
> > > first time the IRQ is used, but not for subsequent usages AFAICT.
> > 
> > I assumed init_one_irq_desc do the work on subsequent usages too. If not,
> > indeed I need to modify creator_domid in few more places.
> 
> I don't think so, init_one_irq_desc will only init the fields if
> handler == NULL, which will only happen the first time the IRQ is
> used, afterwards handler is set to &no_irq_type by destroy_irq.
> 
> Just setting creator_domid = DOMID_INVALID in destroy_irq and adding
> the assert to create_irq should be enough AFAICT, since those
> functions are used exclusively by non-shared IRQs (MSI and MSI-X).

Ok.

> > > >      }
> > > >  
> > > >      spin_lock_irqsave(&desc->lock, flags);
> > > > @@ -2058,7 +2083,7 @@ int map_domain_pirq(
> > > >              spin_unlock_irqrestore(&desc->lock, flags);
> > > >  
> > > >              info = NULL;
> > > > -            irq = create_irq(NUMA_NO_NODE);
> > > > +            irq = create_irq(NUMA_NO_NODE, get_dm_domain(d));
> > > 
> > > Isn't it fine to just use current->domain here directly?
> > > 
> > > It's always going to be the current domain the one that calls
> > > map_domain_pirq in order to get a PIRQ mapped for it's target
> > > domain I think.
> > 
> > I wasn't sure if that's true if all the cases. Especially if hardware
> > domain != toolstack domain. How is it then? Is it hardware domain
> > calling map_domain_pirq in that case?
> 
> But then it's going to be the hardware domain the one that runs the
> QEMU instance, and hence the one that issues the hypercalls to
> map/unmap PIRQs to a target domain?
> 
> ie: the PCI backend (either pciback or QEMU) is not going to run on
> the toolstack domain.

Indeed, you're right. This also means get_dm_domain() helper wouldn't be
needed anymore.

> I'm afraid I don't see a case where current->domain isn't the domain
> also requiring permissions over the IRQ, but I could be wrong. Can you
> come up with a detailed scenario where this might happen?
> 
> > 
> > > >              ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + 
> > > > nr, &info)
> > > >                             : irq;
> > > >              if ( ret < 0 )
> > > > @@ -2691,7 +2716,7 @@ int allocate_and_map_msi_pirq(struct domain *d, 
> > > > int index, int *pirq_p,
> > > >          if ( irq == -1 )
> > > >          {
> > > >      case MAP_PIRQ_TYPE_MULTI_MSI:
> > > > -            irq = create_irq(NUMA_NO_NODE);
> > > > +            irq = create_irq(NUMA_NO_NODE, get_dm_domain(d));
> > > >          }
> > > >  
> > > >          if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> > > > diff --git a/xen/common/irq.c b/xen/common/irq.c
> > > > index f42512d..42b27a9 100644
> > > > --- a/xen/common/irq.c
> > > > +++ b/xen/common/irq.c
> > > > @@ -16,6 +16,7 @@ int init_one_irq_desc(struct irq_desc *desc)
> > > >      spin_lock_init(&desc->lock);
> > > >      cpumask_setall(desc->affinity);
> > > >      INIT_LIST_HEAD(&desc->rl_link);
> > > > +    desc->creator_domid = DOMID_INVALID;
> > > >  
> > > >      err = arch_init_one_irq_desc(desc);
> > > >      if ( err )
> > > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > > > index 189e121..ccc8b04 100644
> > > > --- a/xen/drivers/char/ns16550.c
> > > > +++ b/xen/drivers/char/ns16550.c
> > > > @@ -719,7 +719,7 @@ static void __init ns16550_init_irq(struct 
> > > > serial_port *port)
> > > >      struct ns16550 *uart = port->uart;
> > > >  
> > > >      if ( uart->msi )
> > > > -        uart->irq = create_irq(0);
> > > > +        uart->irq = create_irq(0, NULL);
> > > >  #endif
> > > >  }
> > > >  
> > > > diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
> > > > b/xen/drivers/passthrough/amd/iommu_init.c
> > > > index 4e76b26..50785e0 100644
> > > > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > > > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > > > @@ -781,7 +781,7 @@ static bool_t __init 
> > > > set_iommu_interrupt_handler(struct amd_iommu *iommu)
> > > >      hw_irq_controller *handler;
> > > >      u16 control;
> > > >  
> > > > -    irq = create_irq(NUMA_NO_NODE);
> > > > +    irq = create_irq(NUMA_NO_NODE, NULL);
> > > >      if ( irq <= 0 )
> > > >      {
> > > >          dprintk(XENLOG_ERR, "IOMMU: no irqs\n");
> > > > diff --git a/xen/drivers/passthrough/pci.c 
> > > > b/xen/drivers/passthrough/pci.c
> > > > index e886894..507b3d1 100644
> > > > --- a/xen/drivers/passthrough/pci.c
> > > > +++ b/xen/drivers/passthrough/pci.c
> > > > @@ -845,6 +845,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> > > >      list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> > > >          if ( pdev->bus == bus && pdev->devfn == devfn )
> > > >          {
> > > > +            ret = -EBUSY;
> > > > +            if ( pdev->domain && pdev->domain != hardware_domain )
> > > > +                break;
> > > 
> > > This seems like an unlrelated fix?
> > > 
> > > ie: preventing device removal while in use by a domain different than
> > > dom0?
> > 
> > Indeed it may warrant separate commit now.
> > 
> > > Note that you don't need the pdev->domain != NULL check, just doing
> > > pdev->domain != hardware_domain seems enough, since you don't
> > > dereference the pdev->domain pointer in the expression (unless I'm
> > > missing other usages below).
> > 
> > I don't want to prevent removal if pdev->domain is NULL (if that's even
> > possible).
> 
> But if pdev->domain == NULL, then it's certainly going to be different
> from hardware_domain, 

Exactly. And I do _not_ want to hit that break if pdev->domain == NULL.

> so just using pdev->domain != hardware_domain
> achieves both.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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