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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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

> > >      }
> > >  
> > >      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.

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, so just using pdev->domain != hardware_domain
achieves both.

> > >              ret = iommu_remove_device(pdev);
> > >              if ( pdev->domain )
> > >                  list_del(&pdev->domain_list);
> > > diff --git a/xen/drivers/passthrough/vtd/iommu.c 
> > > b/xen/drivers/passthrough/vtd/iommu.c
> > > index 8b27d7e..79f9682 100644
> > > --- a/xen/drivers/passthrough/vtd/iommu.c
> > > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > > @@ -1138,7 +1138,8 @@ static int __init iommu_set_interrupt(struct 
> > > acpi_drhd_unit *drhd)
> > >      struct irq_desc *desc;
> > >  
> > >      irq = create_irq(rhsa ? pxm_to_node(rhsa->proximity_domain)
> > > -                          : NUMA_NO_NODE);
> > > +                          : NUMA_NO_NODE,
> > > +                     NULL);
> > >      if ( irq <= 0 )
> > >      {
> > >          dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: no irq available!\n");
> > > diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
> > > index c0c6e7c..5b24428 100644
> > > --- a/xen/include/asm-x86/irq.h
> > > +++ b/xen/include/asm-x86/irq.h
> > > @@ -155,7 +155,7 @@ int  init_irq_data(void);
> > >  void clear_irq_vector(int irq);
> > >  
> > >  int irq_to_vector(int irq);
> > > -int create_irq(nodeid_t node);
> > > +int create_irq(nodeid_t node, struct domain *d);
> > >  void destroy_irq(unsigned int irq);
> > >  int assign_irq_vector(int irq, const cpumask_t *);
> > >  
> > > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> > > index 586b783..c7a6a83 100644
> > > --- a/xen/include/xen/irq.h
> > > +++ b/xen/include/xen/irq.h
> > > @@ -91,6 +91,7 @@ typedef struct irq_desc {
> > >      spinlock_t lock;
> > >      struct arch_irq_desc arch;
> > >      cpumask_var_t affinity;
> > > +    domid_t creator_domid; /* weak reference to domain handling this IRQ 
> > > */
> > 
> > I feel like handling is too vague here, but I'm not a native speaker
> > so I'm not sure. I would maybe write:
> > 
> > ... domain having permissions over this IRQ (which can be different
> > from the domain actually having the IRQ assigned) */
> > 
> > Which I think is less ambiguous.
> 
> I wanted to fit the comment in one line. But your version indeed may be
> better.

I would always error towards being more verbose, even if that means
using more that one line. As said, I don't think the comment is wrong,
just feel it could be more detailed.

Thanks, Roger.

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