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

[Shivaprasad G Bhat <sbhat@linux.ibm.com>]

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