[PATCH for 8.0 0/2] virtio-iommu: Fix Replay

[PATCH for 8.0 0/2] virtio-iommu: Fix Replay

[Eric Auger]

When assigning VFIO devices protected by a virtio-iommu we need to replay
the mappings when adding a new IOMMU MR and when attaching a device to
a domain. While we do a "remap" we currently fail to first unmap the
existing IOVA mapping and just map the new one. With some device/group
topology this can lead to errors in VFIO when trying to DMA_MAP IOVA
ranges onto existing ones.

Eric Auger (2):
  virtio-iommu: Add unmap on virtio_iommu_remap()
  virtio-iommu: Fix replay on device attach

 hw/virtio/virtio-iommu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.37.3

[PATCH for 8.0 1/2] virtio-iommu: Add unmap on virtio_iommu_remap()

[Eric Auger]

following replay() callback documentation in memory.h we
shall first invalidate (notify with flag == IOMMU_NONE) and
then map for existing mappings. The code currently skips the
unmap and just do map. This may lead to duplicate mapping
attempts on VFIO side (leading to spurious -EEXIST DMA_MAP
failures). Add the unmap.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Fixes 308e5e1b5f8 ("virtio-iommu: Add replay() memory region callback")
---
 hw/virtio/virtio-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 62e07ec2e4..30334c85aa 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1034,6 +1034,7 @@ static gboolean virtio_iommu_remap(gpointer key, gpointer value, gpointer data)
 
     trace_virtio_iommu_remap(mr->parent_obj.name, interval->low, interval->high,
                              mapping->phys_addr);
+    virtio_iommu_notify_unmap(mr, interval->low, interval->high);
     virtio_iommu_notify_map(mr, interval->low, interval->high,
                             mapping->phys_addr, mapping->flags);
     return false;
-- 
2.37.3

[PATCH for 8.0 2/2] virtio-iommu: Fix replay on device attach

[Eric Auger]

When attaching a device to a domain, we replay the existing
domain mappings for that device. If there are several VFIO
devices in the same group on the guest we may end up with
duplicate mapping attempts because the mapping already exist
on VFIO side. So let's do a proper remap, ie. first unmap
and then map.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Fixes 2f6eeb5f0bb ("virtio-iommu: Call memory notifiers in attach/detach")
---
 hw/virtio/virtio-iommu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 30334c85aa..099dec1f31 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -277,19 +277,21 @@ static gboolean virtio_iommu_notify_unmap_cb(gpointer key, gpointer value,
     return false;
 }
 
-static gboolean virtio_iommu_notify_map_cb(gpointer key, gpointer value,
-                                           gpointer data)
+static gboolean virtio_iommu_notify_remap_cb(gpointer key, gpointer value,
+                                             gpointer data)
 {
     VirtIOIOMMUMapping *mapping = (VirtIOIOMMUMapping *) value;
     VirtIOIOMMUInterval *interval = (VirtIOIOMMUInterval *) key;
     IOMMUMemoryRegion *mr = (IOMMUMemoryRegion *) data;
 
+    virtio_iommu_notify_unmap(mr, interval->low, interval->high);
     virtio_iommu_notify_map(mr, interval->low, interval->high,
                             mapping->phys_addr, mapping->flags);
 
     return false;
 }
 
+
 static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint *ep)
 {
     VirtIOIOMMUDomain *domain = ep->domain;
@@ -489,7 +491,7 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
     virtio_iommu_switch_address_space(sdev);
 
     /* Replay domain mappings on the associated memory region */
-    g_tree_foreach(domain->mappings, virtio_iommu_notify_map_cb,
+    g_tree_foreach(domain->mappings, virtio_iommu_notify_remap_cb,
                    ep->iommu_mr);
 
     return VIRTIO_IOMMU_S_OK;
-- 
2.37.3

Re: [PATCH for 8.0 0/2] virtio-iommu: Fix Replay

[Peter Xu]

Hi, Eric,

On Wed, Dec 07, 2022 at 02:36:44PM +0100, Eric Auger wrote:
> When assigning VFIO devices protected by a virtio-iommu we need to replay
> the mappings when adding a new IOMMU MR and when attaching a device to
> a domain. While we do a "remap" we currently fail to first unmap the
> existing IOVA mapping and just map the new one. With some device/group
> topology this can lead to errors in VFIO when trying to DMA_MAP IOVA
> ranges onto existing ones.

I'm not sure whether virtio-iommu+vfio will suffer from DMA races like when
we were working on the vt-d replay for vfio.  The issue is whether DMA can
happen right after UNMAP but before MAP of the same page if the page was
always mapped.

The vt-d resolved it by using iova_tree so in a replay vt-d knows the page
didn't change, so it avoids unmap+map.  It only notifies newly unmapped or
newly mapped.

Thanks,

-- 
Peter Xu

Re: [PATCH for 8.0 0/2] virtio-iommu: Fix Replay

[Eric Auger]

Hi Peter,

On 12/8/22 00:49, Peter Xu wrote:
> Hi, Eric,
>
> On Wed, Dec 07, 2022 at 02:36:44PM +0100, Eric Auger wrote:
>> When assigning VFIO devices protected by a virtio-iommu we need to replay
>> the mappings when adding a new IOMMU MR and when attaching a device to
>> a domain. While we do a "remap" we currently fail to first unmap the
>> existing IOVA mapping and just map the new one. With some device/group
>> topology this can lead to errors in VFIO when trying to DMA_MAP IOVA
>> ranges onto existing ones.
> I'm not sure whether virtio-iommu+vfio will suffer from DMA races like when
> we were working on the vt-d replay for vfio.  The issue is whether DMA can
> happen right after UNMAP but before MAP of the same page if the page was
> always mapped.

I don't think it can race because a mutex is hold while doing the
virtio_iommu_replay(), and each time a virtio cmd is handled (attach,
map, unmap), see virtio_iommu_handle_command.
So I think it is safe.

Thanks

Eric
>
> The vt-d resolved it by using iova_tree so in a replay vt-d knows the page
> didn't change, so it avoids unmap+map.  It only notifies newly unmapped or
> newly mapped.
>
> Thanks,
>

Re: [PATCH for 8.0 0/2] virtio-iommu: Fix Replay

[Peter Xu]

On Thu, Dec 08, 2022 at 08:48:09AM +0100, Eric Auger wrote:
> Hi Peter,

Hi, Eric,

> 
> On 12/8/22 00:49, Peter Xu wrote:
> > Hi, Eric,
> >
> > On Wed, Dec 07, 2022 at 02:36:44PM +0100, Eric Auger wrote:
> >> When assigning VFIO devices protected by a virtio-iommu we need to replay
> >> the mappings when adding a new IOMMU MR and when attaching a device to
> >> a domain. While we do a "remap" we currently fail to first unmap the
> >> existing IOVA mapping and just map the new one. With some device/group
> >> topology this can lead to errors in VFIO when trying to DMA_MAP IOVA
> >> ranges onto existing ones.
> > I'm not sure whether virtio-iommu+vfio will suffer from DMA races like when
> > we were working on the vt-d replay for vfio.  The issue is whether DMA can
> > happen right after UNMAP but before MAP of the same page if the page was
> > always mapped.
> 
> I don't think it can race because a mutex is hold while doing the
> virtio_iommu_replay(), and each time a virtio cmd is handled (attach,
> map, unmap), see virtio_iommu_handle_command.
> So I think it is safe.

It's not the race in the code, it's the race between modifying host IOMMU
pgtable with DMA happening in parallel.  The bug triggered with DMA_MAP
returning -EEXIST means there's existing mapping.

If during replay there's mapped ranges and the ranges are prone to DMA,
then IIUC it can happen.

I didn't really check specifically for virtio-iommu and I mostly forget the
details, just to raise this up.  It's possible for some reason it just
can't trigger.  VT-d definitely can, in which case we'll see DMA errors on
the host from the assigned device when the DMA triggers during the "unmap
and map" window.

Thanks,

-- 
Peter Xu

Re: [PATCH for 8.0 0/2] virtio-iommu: Fix Replay

[Michael S. Tsirkin]

On Thu, Dec 08, 2022 at 09:58:06AM -0500, Peter Xu wrote:
> On Thu, Dec 08, 2022 at 08:48:09AM +0100, Eric Auger wrote:
> > Hi Peter,
> 
> Hi, Eric,
> 
> > 
> > On 12/8/22 00:49, Peter Xu wrote:
> > > Hi, Eric,
> > >
> > > On Wed, Dec 07, 2022 at 02:36:44PM +0100, Eric Auger wrote:
> > >> When assigning VFIO devices protected by a virtio-iommu we need to replay
> > >> the mappings when adding a new IOMMU MR and when attaching a device to
> > >> a domain. While we do a "remap" we currently fail to first unmap the
> > >> existing IOVA mapping and just map the new one. With some device/group
> > >> topology this can lead to errors in VFIO when trying to DMA_MAP IOVA
> > >> ranges onto existing ones.
> > > I'm not sure whether virtio-iommu+vfio will suffer from DMA races like when
> > > we were working on the vt-d replay for vfio.  The issue is whether DMA can
> > > happen right after UNMAP but before MAP of the same page if the page was
> > > always mapped.
> > 
> > I don't think it can race because a mutex is hold while doing the
> > virtio_iommu_replay(), and each time a virtio cmd is handled (attach,
> > map, unmap), see virtio_iommu_handle_command.
> > So I think it is safe.
> 
> It's not the race in the code, it's the race between modifying host IOMMU
> pgtable with DMA happening in parallel.  The bug triggered with DMA_MAP
> returning -EEXIST means there's existing mapping.
> 
> If during replay there's mapped ranges and the ranges are prone to DMA,
> then IIUC it can happen.
> 
> I didn't really check specifically for virtio-iommu and I mostly forget the
> details, just to raise this up.  It's possible for some reason it just
> can't trigger.  VT-d definitely can, in which case we'll see DMA errors on
> the host from the assigned device when the DMA triggers during the "unmap
> and map" window.
> 
> Thanks,

Eric any resolution on this?

> -- 
> Peter Xu

Re: [PATCH for 8.0 0/2] virtio-iommu: Fix Replay

[Eric Auger]

Hi Michael, Peter,
On 12/20/22 15:59, Michael S. Tsirkin wrote:
> On Thu, Dec 08, 2022 at 09:58:06AM -0500, Peter Xu wrote:
>> On Thu, Dec 08, 2022 at 08:48:09AM +0100, Eric Auger wrote:
>>> Hi Peter,
>> Hi, Eric,
>>
>>> On 12/8/22 00:49, Peter Xu wrote:
>>>> Hi, Eric,
>>>>
>>>> On Wed, Dec 07, 2022 at 02:36:44PM +0100, Eric Auger wrote:
>>>>> When assigning VFIO devices protected by a virtio-iommu we need to replay
>>>>> the mappings when adding a new IOMMU MR and when attaching a device to
>>>>> a domain. While we do a "remap" we currently fail to first unmap the
>>>>> existing IOVA mapping and just map the new one. With some device/group
>>>>> topology this can lead to errors in VFIO when trying to DMA_MAP IOVA
>>>>> ranges onto existing ones.
>>>> I'm not sure whether virtio-iommu+vfio will suffer from DMA races like when
>>>> we were working on the vt-d replay for vfio.  The issue is whether DMA can
>>>> happen right after UNMAP but before MAP of the same page if the page was
>>>> always mapped.
>>> I don't think it can race because a mutex is hold while doing the
>>> virtio_iommu_replay(), and each time a virtio cmd is handled (attach,
>>> map, unmap), see virtio_iommu_handle_command.
>>> So I think it is safe.
>> It's not the race in the code, it's the race between modifying host IOMMU
>> pgtable with DMA happening in parallel.  The bug triggered with DMA_MAP
>> returning -EEXIST means there's existing mapping.
>>
>> If during replay there's mapped ranges and the ranges are prone to DMA,
>> then IIUC it can happen.
>>
>> I didn't really check specifically for virtio-iommu and I mostly forget the
>> details, just to raise this up.  It's possible for some reason it just
>> can't trigger.  VT-d definitely can, in which case we'll see DMA errors on
>> the host from the assigned device when the DMA triggers during the "unmap
>> and map" window.
>>
>> Thanks,
> Eric any resolution on this?

Sorry for the delay. Not yet unfortunately. following Peter's reply I
now understand the race issue and it makes total sense but I need to
study it further.

Eric


>
>> -- 
>> Peter Xu