Re: [for-4.15][PATCH v3 3/3] xen/iommu: x86: Harden the IOMMU page-table allocator

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2021 17:29, Julien Grall wrote:
> On 17/02/2021 15:13, Jan Beulich wrote:
>> On 17.02.2021 15:24, Julien Grall wrote:> --- 
>> 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
>> Nit: s/not/no/ ?
>>
>>> +     * domain is destroyed. Note that arch_iommu_domain_destroy() is
>>> +     * called unconditionally, so pgtables may be unitialized.
>>> +     */
>>> +    ASSERT(dom_iommu(d)->platform_ops == NULL ||
>>> +           page_list_empty(&dom_iommu(d)->arch.pgtables.list));
>>>   }
>>>   
>>>   static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
>>> @@ -279,6 +286,9 @@ int iommu_free_pgtables(struct domain *d)
>>>        */
>>>       hd->platform_ops->clear_root_pgtable(d);
>>>   
>>> +    /* After this barrier no new page allocations can occur. */
>>> +    spin_barrier(&hd->arch.pgtables.lock);
>>
>> Didn't patch 2 utilize the call to ->clear_root_pgtable() itself as
>> the barrier? Why introduce another one (with a similar comment)
>> explicitly now?
> The barriers act differently, one will get against any IOMMU page-tables 
> modification. The other one will gate against allocation.
> 
> There is no guarantee that the former will prevent the latter.

Oh, right - different locks. I got confused here because in both
cases the goal is to prevent allocations.

>>> @@ -315,9 +326,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
>>> +     * reasons to continue to update the IOMMU page-tables while the
>>> +     * 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.
>>> +     */
>>> +    if ( likely(!d->is_dying) )
>>> +    {
>>> +        alive = true;
>>> +        page_list_add(pg, &hd->arch.pgtables.list);
>>> +    }
>>>       spin_unlock(&hd->arch.pgtables.lock);
>>>   
>>> +    if ( unlikely(!alive) )
>>> +    {
>>> +        free_domheap_page(pg);
>>> +        pg = NULL;
>>> +    }
>>> +
>>>       return pg;
>>>   }
>>
>> As before I'm concerned of this forcing error paths to be taken
>> elsewhere, in case an allocation still happens (e.g. from unmap
>> once super page mappings are supported). Considering some of the
>> error handling in the IOMMU code is to invoke domain_crash(), it
>> would be quite unfortunate if we ended up crashing a domain
>> while it is being cleaned up after.
> 
> It is unfortunate, but I think this is better than having to leak page 
> tables.
> 
>>
>> Additionally, the (at present still hypothetical) unmap case, if
>> failing because of the change here, would then again chance to
>> leave mappings in place while the underlying pages get freed. As
>> this would likely require an XSA, the change doesn't feel like
>> "hardening" to me.
> 
> I would agree with this if memory allocations could never fail. That's 
> not that case and will become worse as we use IOMMU pool.
> 
> Do you have callers in mind that doesn't check the returns of iommu_unmap()?

The function is marked __must_check, so there won't be any direct
callers ignoring errors (albeit I may be wrong here - we used to
have cases where we simply suppressed the resulting compiler
diagnostic, without really handling errors; not sure if all of
these are gone by now). Risks might be elsewhere.

Jan