While domain_context_mapping() invokes domain_context_unmap() in a sub-
case of handling DEV_TYPE_PCI when encountering an error, thus avoiding
a leak, individual calls to domain_context_mapping_one() aren't
similarly covered. Such a leak might persist until domain destruction.
Leverage that these cases can be recognized by pdev being non-NULL.

Fixes: dec403cc668f ("VT-d: fix iommu_domid for PCI/PCIx devices assignment")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The Fixes: tag isn't strictly correct, as error handling had more severe
shortcomings at the time. But I wouldn't want to blame a commit
improving error handling to have introduced the leak.

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1518,7 +1518,12 @@ int domain_context_mapping_one(
         rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
 
     if ( rc )
-        domain_context_unmap_one(domain, iommu, bus, devfn);
+    {
+        ret = domain_context_unmap_one(domain, iommu, bus, devfn);
+
+        if ( !ret && pdev && pdev->devfn == devfn )
+            check_cleanup_domid_map(domain, pdev, iommu);
+    }
 
     return rc;
 }

On Fri, Nov 12, 2021 at 10:48:43AM +0100, Jan Beulich wrote:
> While domain_context_mapping() invokes domain_context_unmap() in a sub-
> case of handling DEV_TYPE_PCI when encountering an error, thus avoiding
> a leak, individual calls to domain_context_mapping_one() aren't
> similarly covered. Such a leak might persist until domain destruction.
> Leverage that these cases can be recognized by pdev being non-NULL.

Would it help to place the domid cleanup in domain_context_unmap_one,
as that would then cover calls from domain_context_unmap and the
failure path in domain_context_mapping_one.

Thanks, Roger.

On 12.11.2021 14:42, Roger Pau Monné wrote:
> On Fri, Nov 12, 2021 at 10:48:43AM +0100, Jan Beulich wrote:
>> While domain_context_mapping() invokes domain_context_unmap() in a sub-
>> case of handling DEV_TYPE_PCI when encountering an error, thus avoiding
>> a leak, individual calls to domain_context_mapping_one() aren't
>> similarly covered. Such a leak might persist until domain destruction.
>> Leverage that these cases can be recognized by pdev being non-NULL.
> 
> Would it help to place the domid cleanup in domain_context_unmap_one,
> as that would then cover calls from domain_context_unmap and the
> failure path in domain_context_mapping_one.

I don't think that would work (without further convolution), because of
the up to 3 successive calls in DEV_TYPE_PCI handling. Cleanup may happen
only on the first map's error path or after the last unmap.

Jan

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, November 12, 2021 5:49 PM
> 
> While domain_context_mapping() invokes domain_context_unmap() in a
> sub-
> case of handling DEV_TYPE_PCI when encountering an error, thus avoiding
> a leak, individual calls to domain_context_mapping_one() aren't
> similarly covered. Such a leak might persist until domain destruction.
> Leverage that these cases can be recognized by pdev being non-NULL.
> 
> Fixes: dec403cc668f ("VT-d: fix iommu_domid for PCI/PCIx devices
> assignment")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
> The Fixes: tag isn't strictly correct, as error handling had more severe
> shortcomings at the time. But I wouldn't want to blame a commit
> improving error handling to have introduced the leak.
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1518,7 +1518,12 @@ int domain_context_mapping_one(
>          rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> 
>      if ( rc )
> -        domain_context_unmap_one(domain, iommu, bus, devfn);
> +    {
> +        ret = domain_context_unmap_one(domain, iommu, bus, devfn);
> +
> +        if ( !ret && pdev && pdev->devfn == devfn )
> +            check_cleanup_domid_map(domain, pdev, iommu);
> +    }
> 
>      return rc;
>  }