vhost_vdpa_listener_region_del is always deleting the first iova entry
of the tree, since it's using the needle iova instead of the result's
one.

This was detected using a vga virtual device in the VM using vdpa SVQ.
It makes some extra memory adding and deleting, so the wrong one was
mapped / unmapped. This was undetected before since all the memory was
mappend and unmapped totally without that device, but other conditions
could trigger it too:

* mem_region was with .iova = 0, .translated_addr = (correct GPA).
* iova_tree_find_iova returned right result, but does not update
  mem_region.
* iova_tree_remove always removed region with .iova = 0. Right iova were
  sent to the device.
* Next map will fill the first region with .iova = 0, causing a mapping
  with the same iova and device complains, if the next action is a map.
* Next unmap will cause to try to unmap again iova = 0, causing the
  device to complain that no region was mapped at iova = 0.

Fixes: 34e3c94edaef ("vdpa: Add custom IOTLB translations to SVQ")
Reported-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 291cd19054..00e990ea40 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -290,7 +290,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,

         result = vhost_iova_tree_find_iova(v->iova_tree, &mem_region);
         iova = result->iova;
-        vhost_iova_tree_remove(v->iova_tree, &mem_region);
+        vhost_iova_tree_remove(v->iova_tree, result);
     }
     vhost_vdpa_iotlb_batch_begin_once(v);
     ret = vhost_vdpa_dma_unmap(v, iova, int128_get64(llsize));
--
2.31.1

On Fri, Jul 22, 2022 at 4:26 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> vhost_vdpa_listener_region_del is always deleting the first iova entry
> of the tree, since it's using the needle iova instead of the result's
> one.
>
> This was detected using a vga virtual device in the VM using vdpa SVQ.
> It makes some extra memory adding and deleting, so the wrong one was
> mapped / unmapped. This was undetected before since all the memory was
> mappend and unmapped totally without that device, but other conditions
> could trigger it too:
>
> * mem_region was with .iova = 0, .translated_addr = (correct GPA).
> * iova_tree_find_iova returned right result, but does not update
>   mem_region.
> * iova_tree_remove always removed region with .iova = 0. Right iova were
>   sent to the device.
> * Next map will fill the first region with .iova = 0, causing a mapping
>   with the same iova and device complains, if the next action is a map.
> * Next unmap will cause to try to unmap again iova = 0, causing the
>   device to complain that no region was mapped at iova = 0.
>
> Fixes: 34e3c94edaef ("vdpa: Add custom IOTLB translations to SVQ")
> Reported-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

> ---
>  hw/virtio/vhost-vdpa.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 291cd19054..00e990ea40 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -290,7 +290,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
>
>          result = vhost_iova_tree_find_iova(v->iova_tree, &mem_region);
>          iova = result->iova;
> -        vhost_iova_tree_remove(v->iova_tree, &mem_region);
> +        vhost_iova_tree_remove(v->iova_tree, result);
>      }
>      vhost_vdpa_iotlb_batch_begin_once(v);
>      ret = vhost_vdpa_dma_unmap(v, iova, int128_get64(llsize));
> --
> 2.31.1
>

On Mon, Jul 25, 2022 at 3:05 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jul 22, 2022 at 4:26 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > vhost_vdpa_listener_region_del is always deleting the first iova entry
> > of the tree, since it's using the needle iova instead of the result's
> > one.
> >
> > This was detected using a vga virtual device in the VM using vdpa SVQ.
> > It makes some extra memory adding and deleting, so the wrong one was
> > mapped / unmapped. This was undetected before since all the memory was
> > mappend and unmapped totally without that device, but other conditions
> > could trigger it too:
> >
> > * mem_region was with .iova = 0, .translated_addr = (correct GPA).
> > * iova_tree_find_iova returned right result, but does not update
> >   mem_region.
> > * iova_tree_remove always removed region with .iova = 0. Right iova were
> >   sent to the device.
> > * Next map will fill the first region with .iova = 0, causing a mapping
> >   with the same iova and device complains, if the next action is a map.
> > * Next unmap will cause to try to unmap again iova = 0, causing the
> >   device to complain that no region was mapped at iova = 0.
> >
> > Fixes: 34e3c94edaef ("vdpa: Add custom IOTLB translations to SVQ")
> > Reported-by: Lei Yang <leiyang@redhat.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
> Acked-by: Jason Wang <jasowang@redhat.com>

I've queued this for hard-freeze.

Thanks

>
> > ---
> >  hw/virtio/vhost-vdpa.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 291cd19054..00e990ea40 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -290,7 +290,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
> >
> >          result = vhost_iova_tree_find_iova(v->iova_tree, &mem_region);
> >          iova = result->iova;
> > -        vhost_iova_tree_remove(v->iova_tree, &mem_region);
> > +        vhost_iova_tree_remove(v->iova_tree, result);
> >      }
> >      vhost_vdpa_iotlb_batch_begin_once(v);
> >      ret = vhost_vdpa_dma_unmap(v, iova, int128_get64(llsize));
> > --
> > 2.31.1
> >