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, insert the container object into the address
space's container list earlier, just after it is created.

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>
---
 hw/vfio/container.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 9ccdb639ac84f885da40eace8a0059f397295619..b42466701bf13818b538483ec4c143ce6f83c598 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -618,6 +618,8 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     }
     bcontainer = &container->bcontainer;
 
+    vfio_address_space_insert(space, bcontainer);
+
     if (!vfio_cpr_register_container(bcontainer, errp)) {
         goto free_container_exit;
     }
@@ -637,8 +639,6 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
 
     vfio_kvm_device_add_group(group);
 
-    vfio_address_space_insert(space, bcontainer);
-
     group->container = container;
     QLIST_INSERT_HEAD(&container->group_list, group, container_next);
 
-- 
2.47.0

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Friday, November 15, 2024 2:53 PM
>Subject: [PATCH for-9.2] 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, insert the container object into the address
>space's container list earlier, just after it is created.

It looks we still have issue if create a container then release it right away?
If that's true, I would suggest to also use QLIST_SAFE_REMOVE for
QLIST_REMOVE(bcontainer, next) besides this change.

Thanks
Zhenzhong

>
>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>
>---
> hw/vfio/container.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>index
>9ccdb639ac84f885da40eace8a0059f397295619..b42466701bf13818b538483ec4
>c143ce6f83c598 100644
>--- a/hw/vfio/container.c
>+++ b/hw/vfio/container.c
>@@ -618,6 +618,8 @@ static bool vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>     }
>     bcontainer = &container->bcontainer;
>
>+    vfio_address_space_insert(space, bcontainer);
>+
>     if (!vfio_cpr_register_container(bcontainer, errp)) {
>         goto free_container_exit;
>     }
>@@ -637,8 +639,6 @@ static bool vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>
>     vfio_kvm_device_add_group(group);
>
>-    vfio_address_space_insert(space, bcontainer);
>-
>     group->container = container;
>     QLIST_INSERT_HEAD(&container->group_list, group, container_next);
>
>--
>2.47.0

Zhenzhong

On 11/15/24 08:45, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Friday, November 15, 2024 2:53 PM
>> Subject: [PATCH for-9.2] 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, insert the container object into the address
>> space's container list earlier, just after it is created.
> 
> It looks we still have issue if create a container then release it right away?

There is a small window indeed in the sequence. It is not an issue
today because there are no error handling path which would destroy
the object but it could be changed in the future.

I think we should move the insertion of the container in the container
list of the address space in vfio_create_container() after the call
to VFIO_GROUP_SET_CONTAINER. With the change you proposed below, we
care less.

> If that's true, I would suggest to also use QLIST_SAFE_REMOVE for
> QLIST_REMOVE(bcontainer, next) besides this change.

Oh. I was not aware of version of QLIST_REMOVE. Yes, let's do that
for 9.2.

Thanks,

C.