Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Shivaprasad G Bhat]

On 5/13/24 17:53, Cédric Le Goater wrote:
> 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.
Sure.
> 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 requires the VFIOContainer to be a DeviceState object.

The existing base class TYPE_VFIO_IOMMU is an InterfaceClass.

I attempted VFIOContainer object declaration with TYPE_VFIO_IOMMU,

like

OBJECT_DECLARE_SIMPLE_TYPE(VFIOContainer, VFIO_IOMMU_LEGACY)


> 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.


With my above changes,

I see the instance_init() is not supported for the InterfaceClass with the

checks from below

commit 422ca1432f7b44f2a9f3ad94a65d36927da021fa
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Wed Sep 12 16:53:03 2018 +0400

    qom/object: add some interface asserts

Did you suggest me something else?


Thank you,

Shivaprasad

> 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;