vfio: container: Fix missing allocation of VFIOSpaprContainer

The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
spapr container)" began to use the newly introduced VFIOSpaprContainer
structure.

After several refactors, today the container_of(container,
VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
not allocated. On PPC64 systems, this dereference is leading to corruption
showing up as glibc malloc assertion during guest start when using vfio.

Patch adds the missing allocation while also making the structure movement
to vfio common header file.

Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 hw/vfio/container.c           |    6 ++++--
 hw/vfio/spapr.c               |    6 ------
 include/hw/vfio/vfio-common.h |    6 ++++++
 3 files changed, 10 insertions(+), 8 deletions(-)

>-----Original Message-----
>From: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>Subject: [PATCH] vfio: container: Fix missing allocation of
>VFIOSpaprContainer
>
>The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
>spapr container)" began to use the newly introduced VFIOSpaprContainer
>structure.
>
>After several refactors, today the container_of(container,
>VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
>not allocated. On PPC64 systems, this dereference is leading to corruption
>showing up as glibc malloc assertion during guest start when using vfio.
>
>Patch adds the missing allocation while also making the structure movement
>to vfio common header file.
>
>Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
>Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>

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

An alternative way is to introduce a VFIOIOMMUClass::create or
VFIOIOMMUClass::get_container_size.
But that needs some refactor to vfio_connect_container().

Thanks
Zhenzhong

>---
> hw/vfio/container.c           |    6 ++++--
> hw/vfio/spapr.c               |    6 ------
> include/hw/vfio/vfio-common.h |    6 ++++++
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
>diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>index 77bdec276e..ecaf5786d9 100644
>--- a/hw/vfio/container.c
>+++ b/hw/vfio/container.c
>@@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
> {
>     VFIOContainer *container;
>     VFIOContainerBase *bcontainer;
>+    VFIOSpaprContainer *scontainer;
>     int ret, fd;
>     VFIOAddressSpace *space;
>
>@@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>         goto close_fd_exit;
>     }
>
>-    container = g_malloc0(sizeof(*container));
>+    scontainer = g_malloc0(sizeof(*scontainer));
>+    container = &scontainer->container;
>     container->fd = fd;
>     bcontainer = &container->bcontainer;
>
>@@ -675,7 +677,7 @@ unregister_container_exit:
>     vfio_cpr_unregister_container(bcontainer);
>
> free_container_exit:
>-    g_free(container);
>+    g_free(scontainer);
>
> close_fd_exit:
>     close(fd);
>diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
>index 0d949bb728..78d218b7e7 100644
>--- a/hw/vfio/spapr.c
>+++ b/hw/vfio/spapr.c
>@@ -24,12 +24,6 @@
> #include "qapi/error.h"
> #include "trace.h"
>
>-typedef struct VFIOSpaprContainer {
>-    VFIOContainer container;
>-    MemoryListener prereg_listener;
>-    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>-} VFIOSpaprContainer;
>-
> static bool vfio_prereg_listener_skipped_section(MemoryRegionSection
>*section)
> {
>     if (memory_region_is_iommu(section->mr)) {
>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>index b9da6c08ef..010fa68ac6 100644
>--- a/include/hw/vfio/vfio-common.h
>+++ b/include/hw/vfio/vfio-common.h
>@@ -82,6 +82,12 @@ typedef struct VFIOContainer {
>     QLIST_HEAD(, VFIOGroup) group_list;
> } VFIOContainer;
>
>+typedef struct VFIOSpaprContainer {
>+    VFIOContainer container;
>+    MemoryListener prereg_listener;
>+    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>+} VFIOSpaprContainer;
>+
> typedef struct VFIOHostDMAWindow {
>     hwaddr min_iova;
>     hwaddr max_iova;
>

Hello Shivaprasad,

On 5/9/24 21:14, Shivaprasad G Bhat wrote:
> The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
> spapr container)" began to use the newly introduced VFIOSpaprContainer
> structure.
> 
> After several refactors, today the container_of(container,
> VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
> not allocated. On PPC64 systems, this dereference is leading to corruption
> showing up as glibc malloc assertion during guest start when using vfio.
> 
> Patch adds the missing allocation while also making the structure movement
> to vfio common header file.
> 
> Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>   hw/vfio/container.c           |    6 ++++--
>   hw/vfio/spapr.c               |    6 ------
>   include/hw/vfio/vfio-common.h |    6 ++++++
>   3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 77bdec276e..ecaf5786d9 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>   {
>       VFIOContainer *container;
>       VFIOContainerBase *bcontainer;
> +    VFIOSpaprContainer *scontainer;

We should do our best to avoid any direct use of ppc related attributes
in the common VFIO code. This comment also applies to VFIO_SPAPR_TCE*
which are still there because the clean up is not finished. So, this
proposal will have to be reworked.

The first step is to finish the QOMification of VFIOContainer, so
that the VFIOContainer instance is created in vfio_connect_container()
with :

     container = qdev_new(iommu_type_name);

This means reworking this part (and vfio_set_iommu()) :

     ...
     container = g_malloc0(sizeof(*container));
     container->fd = fd;
     bcontainer = &container->bcontainer;

     if (!vfio_set_iommu(container, group->fd, space, errp)) {
         goto free_container_exit;
     }
     ...

VFIOSpaprContainer can then implement its own .init_instance() handler
to allocate/initialize attributes required by the pseries machines.

While doing this, please try to reduce the use of ->iommu_type which
is a design shortcut. I would like to completely remove it at some
point.

Thanks,

C.







>       int ret, fd;
>       VFIOAddressSpace *space;
> 
> @@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>           goto close_fd_exit;
>       }
> 
> -    container = g_malloc0(sizeof(*container));
> +    scontainer = g_malloc0(sizeof(*scontainer));
> +    container = &scontainer->container;
>       container->fd = fd;
>       bcontainer = &container->bcontainer;
> 
> @@ -675,7 +677,7 @@ unregister_container_exit:
>       vfio_cpr_unregister_container(bcontainer);
> 
>   free_container_exit:
> -    g_free(container);
> +    g_free(scontainer);
> 
>   close_fd_exit:
>       close(fd);
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 0d949bb728..78d218b7e7 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -24,12 +24,6 @@
>   #include "qapi/error.h"
>   #include "trace.h"
> 
> -typedef struct VFIOSpaprContainer {
> -    VFIOContainer container;
> -    MemoryListener prereg_listener;
> -    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> -} VFIOSpaprContainer;
> -
>   static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
>   {
>       if (memory_region_is_iommu(section->mr)) {
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef..010fa68ac6 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -82,6 +82,12 @@ typedef struct VFIOContainer {
>       QLIST_HEAD(, VFIOGroup) group_list;
>   } VFIOContainer;
> 
> +typedef struct VFIOSpaprContainer {
> +    VFIOContainer container;
> +    MemoryListener prereg_listener;
> +    QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> +} VFIOSpaprContainer;
> +
>   typedef struct VFIOHostDMAWindow {
>       hwaddr min_iova;
>       hwaddr max_iova;
> 
>