[PATCH v2] vfio/container: Fix container object destruction

[PATCH v2] vfio/container: Fix container object destruction

[Cédric Le Goater]

When commit 96b7af4388b3 intoduced a .instance_finalize() handler,
it did not take into account that the container was not necessarily
inserted into the container list of the address space. Hence, if
the container object is destroyed, by calling object_unref() for
example, before vfio_address_space_insert() is called, QEMU may
crash when removing the container from the list as done in
vfio_container_instance_finalize(). This was seen with an SEV-SNP
guest for which discarding of RAM fails.

To resolve this issue, use the safe version of QLIST_REMOVE().

Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
Cc: Eric Auger <eric.auger@redhat.com>
Fixes: 96b7af4388b3 ("vfio/container: Move vfio_container_destroy() to an instance_finalize() handler")
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---

 Changes in v2:

 - use the safe version of QLIST_REMOVE() instead of calling
   vfio_address_space_insert() earlier.

 hw/vfio/container-base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 809b15767425a48f2404b08fc409ee5684af2094..6f86c37d971ec38426dacd471bca837c0d0df806 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -103,7 +103,7 @@ static void vfio_container_instance_finalize(Object *obj)
     VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
     VFIOGuestIOMMU *giommu, *tmp;
 
-    QLIST_REMOVE(bcontainer, next);
+    QLIST_SAFE_REMOVE(bcontainer, next);
 
     QLIST_FOREACH_SAFE(giommu, &bcontainer->giommu_list, giommu_next, tmp) {
         memory_region_unregister_iommu_notifier(
-- 
2.47.0

RE: [PATCH v2] vfio/container: Fix container object destruction

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Friday, November 15, 2024 4:35 PM
>Subject: [PATCH v2] vfio/container: Fix container object destruction
>
>When commit 96b7af4388b3 intoduced a .instance_finalize() handler,
>it did not take into account that the container was not necessarily
>inserted into the container list of the address space. Hence, if
>the container object is destroyed, by calling object_unref() for
>example, before vfio_address_space_insert() is called, QEMU may
>crash when removing the container from the list as done in
>vfio_container_instance_finalize(). This was seen with an SEV-SNP
>guest for which discarding of RAM fails.
>
>To resolve this issue, use the safe version of QLIST_REMOVE().
>
>Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
>Cc: Eric Auger <eric.auger@redhat.com>
>Fixes: 96b7af4388b3 ("vfio/container: Move vfio_container_destroy() to an
>instance_finalize() handler")
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

Re: [PATCH v2] vfio/container: Fix container object destruction

[Eric Auger]

Hi Cédric,

On 11/15/24 09:34, Cédric Le Goater wrote:
> When commit 96b7af4388b3 intoduced a .instance_finalize() handler,
> it did not take into account that the container was not necessarily
> inserted into the container list of the address space. Hence, if
> the container object is destroyed, by calling object_unref() for
> example, before vfio_address_space_insert() is called, QEMU may
> crash when removing the container from the list as done in
> vfio_container_instance_finalize(). This was seen with an SEV-SNP
> guest for which discarding of RAM fails.
>
> To resolve this issue, use the safe version of QLIST_REMOVE().
>
> Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Fixes: 96b7af4388b3 ("vfio/container: Move vfio_container_destroy() to an instance_finalize() handler")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>
>  Changes in v2:
>
>  - use the safe version of QLIST_REMOVE() instead of calling
>    vfio_address_space_insert() earlier.
>
>  hw/vfio/container-base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 809b15767425a48f2404b08fc409ee5684af2094..6f86c37d971ec38426dacd471bca837c0d0df806 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -103,7 +103,7 @@ static void vfio_container_instance_finalize(Object *obj)
>      VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
>      VFIOGuestIOMMU *giommu, *tmp;
>  
> -    QLIST_REMOVE(bcontainer, next);
> +    QLIST_SAFE_REMOVE(bcontainer, next);
>  
>      QLIST_FOREACH_SAFE(giommu, &bcontainer->giommu_list, giommu_next, tmp) {
>          memory_region_unregister_iommu_notifier(