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

["Cédric Le Goater" <clg@redhat.com>]

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