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

[Julien Grall <julien@xxxxxxx>]

On 23/12/2020 14:34, Jan Beulich wrote:
> On 22.12.2020 16:43, 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 in two
> > cases:
> >      1) domain_relinquish_resources() is not called when the domain
> >      creation fails.
> Could you remind me of what IOMMU page table insertions there
> are during domain creation? No memory got allocated to the
> domain at that point yet, so it would seem to me there simply
> is nothing to map.
The P2M is first modified in hvm_domain_initialise():

(XEN) Xen call trace:
(XEN)    [<ffff82d04026b9ec>] R iommu_alloc_pgtable+0x11/0x137
(XEN)    [<ffff82d04025f9f5>] F 
drivers/passthrough/vtd/iommu.c#addr_to_dma_page_maddr+0x146/0x1d8
(XEN)    [<ffff82d04025fcc5>] F 
drivers/passthrough/vtd/iommu.c#intel_iommu_map_page+0x6a/0x14b
(XEN)    [<ffff82d04026d949>] F iommu_map+0x6d/0x16f
(XEN)    [<ffff82d04026da71>] F iommu_legacy_map+0x26/0x63
(XEN)    [<ffff82d040301bdc>] F 
arch/x86/mm/p2m-ept.c#ept_set_entry+0x6b2/0x730
(XEN)    [<ffff82d0402f67e7>] F p2m_set_entry+0x91/0x128
(XEN)    [<ffff82d0402f6b5c>] F 
arch/x86/mm/p2m.c#set_typed_p2m_entry+0xfe/0x3f7
(XEN)    [<ffff82d0402f7f4c>] F set_mmio_p2m_entry+0x65/0x6e
(XEN)    [<ffff82d04029a080>] F 
arch/x86/hvm/vmx/vmx.c#vmx_domain_initialise+0xf6/0x137
(XEN)    [<ffff82d0402af421>] F hvm_domain_initialise+0x357/0x4c7
(XEN)    [<ffff82d04031eae7>] F arch_domain_create+0x478/0x4ff
(XEN)    [<ffff82d04020476e>] F domain_create+0x4f2/0x778
(XEN)    [<ffff82d04023b0d2>] F do_domctl+0xb1e/0x18b8
(XEN)    [<ffff82d040311dbf>] F pv_hypercall+0x2f0/0x55f
(XEN)    [<ffff82d040390432>] F lstar_enter+0x112/0x120

> >      2) There is nothing preventing page-table allocations when the
> >      domain is dying.
> >
> > In both cases, this can be solved by freeing the page-tables again
> > when the domain destruction. Although, this may result to an high
> > number of page-tables to free.
> Since I've seen this before in this series, and despite me also
> not being a native speaker, as a nit: I don't think it can
> typically be other than "result in".
I think you are right.

> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -2290,7 +2290,7 @@ int domain_relinquish_resources(struct domain *d)
> >   
> >       PROGRESS(iommu_pagetables):
> >   
> > -        ret = iommu_free_pgtables(d);
> > +        ret = iommu_free_pgtables(d, false);
> I suppose you mean "true" here, but I also think the other
> approach (checking for DOMDYING_dead, which you don't seem to
> like very much) is better, if for no other reason than it
> already being used elsewhere.
I think "don't like very much" is an understatement :). There seems to 
be more function using an extra parameter (such as hap_set_allocation() 
which was introduced before your DOMDYING_dead). So I only followed what 
they did.
> > @@ -305,6 +320,19 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
> >           memflags = MEMF_node(hd->node);
> >   #endif
> >   
> > +    /*
> > +     * The IOMMU page-tables are freed when relinquishing the domain, but
> > +     * nothing prevent allocation to happen afterwards. There is no valid
> > +     * reasons to continue to update the IOMMU page-tables while the
> > +     * domain is dying.
> > +     *
> > +     * So prevent page-table allocation when the domain is dying. Note
> > +     * this doesn't fully prevent the race because d->is_dying may not
> > +     * yet be seen.
> > +     */
> > +    if ( d->is_dying )
> > +        return NULL;
> > +
> >       pg = alloc_domheap_page(NULL, memflags);
> >       if ( !pg )
> >           return NULL;
> As said in reply to an earlier patch - with a suitable
> spin_barrier() you can place your check further down, along the
> lines of
>
>      spin_lock(&hd->arch.pgtables.lock);
>      if ( likely(!d->is_dying) )
>      {
>          page_list_add(pg, &hd->arch.pgtables.list);
>          p = NULL;
>      }
>      spin_unlock(&hd->arch.pgtables.lock);
>
>      if ( p )
>      {
>          free_domheap_page(pg);
>          pg = NULL;
>      }
>
> (albeit I'm relatively sure you won't like the re-purposing of
> p, but that's a minor detail). (FREE_DOMHEAP_PAGE() would be
> nice to use here, but we seem to only have FREE_XENHEAP_PAGE()
> so far.)
In fact I don't mind the re-purposing of p. However, I dislike the 
allocation and then freeing when the domain is dying.
I think I prefer the small race introduced (the pages will still be 
freed) over this solution.
Note that Paul's IOMMU series will completely rework the function. So 
this is only temporary.
Cheers,

--
Julien Grall