Re: [for-4.15][PATCH v2 4/5] xen/iommu: x86: Don't leak the IOMMU page-tables

[Julien Grall <julien@xxxxxxx>]

On 10/02/2021 14:32, Jan Beulich wrote:
> On 09.02.2021 16:28, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > The new IOMMU page-tables allocator will release the pages when
> > relinquish the domain resources. However, this is not sufficient when
> > the domain is dying because nothing prevents page-table to be allocated.
> >
> > iommu_alloc_pgtable() is now checking if the domain is dying before
> > adding the page in the list. We are relying on &hd->arch.pgtables.lock
> > to synchronize d->is_dying.
> As said in reply to an earlier patch, I think suppressing
> (really: ignoring) new mappings would be better.
This is exactly what I suggested in v1 but you wrote:

"Ignoring requests there seems fragile to me. Paul - what are your
thoughts about bailing early from hvm_add_ioreq_gfn() when the
domain is dying?"

Are you know saying that the following snipped would be fine:

if ( d->is_dying )
  return 0;

> You could
> utilize the same lock, but you'd need to duplicate the
> checking in {amd,intel}_iommu_map_page().
>
> I'm not entirely certain in the case about unmap requests:
> It may be possible to also suppress/ignore them, but this
> may require some further thought.
I think the unmap part is quite risky to d->is_dying because the PCI 
devices may not quiesced and still assigned to the domain.
> Apart from this, just in case we settle on your current
> approach, a few spelling nits:
>
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -149,6 +149,13 @@ int arch_iommu_domain_init(struct domain *d)
> >   
> >   void arch_iommu_domain_destroy(struct domain *d)
> >   {
> > +    /*
> > +     * There should be not page-tables left allocated by the time the
> ... should be no ...
>
> > +     * domain is destroyed. Note that arch_iommu_domain_destroy() is
> > +     * called unconditionally, so pgtables may be unitialized.
> uninitialized
>
> > @@ -303,9 +317,29 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
> >       unmap_domain_page(p);
> >   
> >       spin_lock(&hd->arch.pgtables.lock);
> > -    page_list_add(pg, &hd->arch.pgtables.list);
> > +    /*
> > +     * The IOMMU page-tables are freed when relinquishing the domain, but
> > +     * nothing prevent allocation to happen afterwards. There is no valid
> prevents
>
> > +     * reasons to continue to update the IOMMU page-tables while the
> reason
>
> > +     * domain is dying.
> > +     *
> > +     * So prevent page-table allocation when the domain is dying.
> > +     *
> > +     * We relying on &hd->arch.pgtables.lock to synchronize d->is_dying.
> rely
>
> Jan
--
Julien Grall