Message ID | 20241115065240.2198493-1-clg@redhat.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [for-9.2] vfio/container: Fix container object destruction | expand |
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.
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);
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(-)