[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

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

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

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

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.