| Message ID | fcc51df9-0896-247b-d4ad-0de4db6c2a9c@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VT-d: address fallout from XSA-400 | expand |
On Thu, Apr 07, 2022 at 08:11:45AM +0200, Jan Beulich wrote: > 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. I wonder whether it would be cleaner to stash the previous context entry if present and try to (re)set that one instead of recurring into ourselves. > Fixes: 8f41e481b485 ("VT-d: re-assign devices directly") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 07.04.2022 09:51, Roger Pau Monné wrote: > On Thu, Apr 07, 2022 at 08:11:45AM +0200, Jan Beulich wrote: >> 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. > > I wonder whether it would be cleaner to stash the previous context > entry if present and try to (re)set that one instead of recurring into > ourselves. I'm not sure this would be cleaner (it might be easier): The entry may have had modifications which we want to undo by a clean establishing of the "new" (really original) mapping. Otoh roll-back can certainly mean simply going back to what was there. But that would likely want to be a separate change, for likely coming with a lot of code churn: I'd see the function gaining a two-iteration loop then, which would mean re-indenting fair parts of it. But maybe it could also be dealt with by other means, while still not introducing a fake loop via adding a "goto" back to the top of the function ... >> Fixes: 8f41e481b485 ("VT-d: re-assign devices directly") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan
> From: Jan Beulich <jbeulich@suse.com> > 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@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > 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
--- 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
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@suse.com> --- v2: Extend scope of the approach taken. Leverage for some cleanup.