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

[Marek Marczykowski <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 27, 2019 at 04:07:54AM -0700, Jan Beulich wrote:
> >>> On 08.02.19 at 11:17, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > There is one code path where I haven't managed to properly extract
> > possible stubdomain in use:
> > pci_remove_device()
> >  -> pci_cleanup_msi()
> >    -> msi_free_irqs()
> >      -> msi_free_irq()
> >        -> destroy_irq()
> > 
> > For now I've hardcoded hardware_domain there (in msi_free_irqs). Can it 
> > happen
> > when device is still assigned to some domU?
> 
> In case this question is still open: No, it can't with current code,
> and provided Dom0 behaves correctly.

Thanks for confirmation.

> > @@ -368,13 +369,13 @@ static int __init hpet_assign_irq(struct 
> > hpet_event_channel *ch)
> >  {
> >      int irq;
> >  
> > -    if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
> > +    if ( (irq = create_irq(NUMA_NO_NODE, hardware_domain)) < 0 )
> >          return irq;
> >  
> >      ch->msi.irq = irq;
> >      if ( hpet_setup_msi_irq(ch) )
> >      {
> > -        destroy_irq(irq);
> > +        destroy_irq(irq, hardware_domain);
> >          return -EINVAL;
> >      }
> 
> Why don't you take the opportunity here (and elsewhere) and properly
> remove hwdom access to such internal-to-Xen IRQs? Simply pass NULL
> here, and skip permission granting in this case (create_irq() already
> checks for NULL anyway).

Already queued for v5, per Roger's review.

> > @@ -190,19 +190,19 @@ int create_irq(nodeid_t node)
> >          desc->arch.used = IRQ_UNUSED;
> >          irq = ret;
> >      }
> > -    else if ( hardware_domain )
> > +    else if ( dm_domain )
> >      {
> > -        ret = irq_permit_access(hardware_domain, irq);
> > +        ret = irq_permit_access(dm_domain, irq);
> 
> Doesn't this imply that Dom0 has no way of cleaning up after the
> guest/stubdom pair? IOW I wonder whether both dm and hwdom
> should be granted access.

See discussion with Roger on this very patch.
In short: since permissions are stored in domain struct, not irq, there
is not much to cleanup after domain destruction. Also, toolstack in dom0
has no idea about IRQs allocated by stubdomain, so it couldn't do such
cleanup anyway.

> > @@ -2095,7 +2099,9 @@ int map_domain_pirq(
> >                  irq = info->arch.irq;
> >              }
> >              msi_desc->irq = -1;
> > -            msi_free_irq(msi_desc);
> > +            msi_free_irq(msi_desc,
> > +                         current->domain->target == d ? current->domain
> > +                                                      : hardware_domain);
> 
> Note how ->irq gets set to -1 prior to the call (and also in at least
> one other instance), which will lead to skipping of the destroy_irq()
> call, and hence skipping of the permission removal. Or wait, that's
> going to be taken care of in the caller as it seems. If this is also
> your understanding, then please add a sentence to the description
> pointing this out. The split logic isn't really helpful here (I know it
> was me who wrote it, in an attempt to avoid re-writing everything
> basically from scratch).

Yes, that matches my understanding - the caller will call on error
destroy_irq(), if it called create_irq() before (which may not always be
the case - and I think this is why it isn't destroyed here).

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