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

[Cédric Le Goater]

On 5/22/24 18:15, Shivaprasad G Bhat wrote:
> 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


Yes. We need an Object, hence my remark on "QOMification of VFIOContainer".
VFIOContainerBase needs to be reworked.

Thanks,

C.

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