RE: [PATCH v2 2/2] VT-d: avoid infinite recursion on domain_context_mapping_one() error path

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, April 7, 2022 2:12 PM
> 
> Despite the comment there infinite recursion was still possible, by
> flip-flopping between two domains. This is because prev_dom is derived
> from the DID found in the context entry, which was already updated by
> the time error recovery is invoked. Simply introduce yet another mode
> flag to prevent rolling back an in-progress roll-back of a prior
> mapping attempt.
> 
> Also drop the existing recursion prevention for having been dead anyway:
> Earlier in the function we already bail when prev_dom == domain.
> 
> Fixes: 8f41e481b485 ("VT-d: re-assign devices directly")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

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

> ---
> v2: Extend scope of the approach taken. Leverage for some cleanup.
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1599,7 +1599,7 @@ int domain_context_mapping_one(
>      if ( !seg && !rc )
>          rc = me_wifi_quirk(domain, bus, devfn, domid, pgd_maddr, mode);
> 
> -    if ( rc )
> +    if ( rc && !(mode & MAP_ERROR_RECOVERY) )
>      {
>          if ( !prev_dom ||
>               /*
> @@ -1609,13 +1609,12 @@ int domain_context_mapping_one(
>                */
>               (prev_dom == dom_io && !pdev) )
>              ret = domain_context_unmap_one(domain, iommu, bus, devfn);
> -        else if ( prev_dom != domain ) /* Avoid infinite recursion. */
> +        else
>              ret = domain_context_mapping_one(prev_dom, iommu, bus, devfn,
> pdev,
>                                               DEVICE_DOMID(prev_dom, pdev),
>                                               DEVICE_PGTABLE(prev_dom, pdev),
> -                                             mode & MAP_WITH_RMRR) < 0;
> -        else
> -            ret = 1;
> +                                             (mode & MAP_WITH_RMRR) |
> +                                             MAP_ERROR_RECOVERY) < 0;
> 
>          if ( !ret && pdev && pdev->devfn == devfn )
>              check_cleanup_domid_map(domain, pdev, iommu);
> --- a/xen/drivers/passthrough/vtd/vtd.h
> +++ b/xen/drivers/passthrough/vtd/vtd.h
> @@ -29,7 +29,8 @@
>  #define MAP_WITH_RMRR         (1u << 0)
>  #define MAP_OWNER_DYING       (1u << 1)
>  #define MAP_SINGLE_DEVICE     (1u << 2)
> -#define UNMAP_ME_PHANTOM_FUNC (1u << 3)
> +#define MAP_ERROR_RECOVERY    (1u << 3)
> +#define UNMAP_ME_PHANTOM_FUNC (1u << 4)
> 
>  /* Allow for both IOAPIC and IOSAPIC. */
>  #define IO_xAPIC_route_entry IO_APIC_route_entry