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

From: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Simon Gaiser <simon@invisiblethingslab.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org, Brian Woods <brian.woods@amd.com>
Subject: Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
Date: Thu, 18 Jul 2019 17:12:54 +0200	[thread overview]
Message-ID: <20190718151254.GZ1250@mail-itl> (raw)
In-Reply-To: <20190718092939.2m3di3dtspsku6jk@Air-de-Roger.citrite.net>

[-- Attachment #1.1: Type: text/plain, Size: 7521 bytes --]

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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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