Re: [PATCH 3/4] vfio/container: Fix potential SIGSEGV when recover from unmap-all-vaddr failure

[Steven Sistare <steven.sistare@oracle.com>]

On 6/24/2025 12:54 PM, Cédric Le Goater wrote:
> On 6/23/25 12:22, Zhenzhong Duan wrote:
>> cpr.saved_dma_map isn't initialized in source qemu which lead to vioc->dma_map
>> assigned a NULL value, this will trigger SIGSEGV.
> 
> I don't understand the scenario. Could you please explain more ?

Thank you Zhenzhong, I see the bug.

CPR overrides then restores dma_map in both outgoing and incoming QEMU, for
different reasons. But it only sets saved_dma_map in the target.

So, a more future-proof fix IMO is to always set saved_dma_map:

@@ -182,9 +182,9 @@ bool vfio_legacy_cpr_register_container(VFIOContainer *conta
      vmstate_register(NULL, -1, &vfio_container_vmstate, container);

      /* During incoming CPR, divert calls to dma_map. */
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+    container->cpr.saved_dma_map = vioc->dma_map;
      if (cpr_is_incoming()) {
-        VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
-        container->cpr.saved_dma_map = vioc->dma_map;
          vioc->dma_map = vfio_legacy_cpr_dma_map;
      }

- Steve

>> Fix it by save and restore vioc->dma_map locally.
> 
> Steve, this is cpr territory. Is it still an issue with the rest
> of the patches applied ?
>  
> Thanks,
> 
> C.
> 
>> Fixes: eba1f657cbb1 ("vfio/container: recover from unmap-all-vaddr failure")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-cpr.h | 8 +++++---
>>   hw/vfio/cpr-legacy.c       | 3 ++-
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
>> index 8bf85b9f4e..aef542e93c 100644
>> --- a/include/hw/vfio/vfio-cpr.h
>> +++ b/include/hw/vfio/vfio-cpr.h
>> @@ -16,14 +16,16 @@ struct VFIOContainer;
>>   struct VFIOContainerBase;
>>   struct VFIOGroup;
>> +typedef int (*DMA_MAP_FUNC)(const struct VFIOContainerBase *bcontainer,
>> +                            hwaddr iova, ram_addr_t size, void *vaddr,
>> +                            bool readonly, MemoryRegion *mr);
>> +
>>   typedef struct VFIOContainerCPR {
>>       Error *blocker;
>>       bool vaddr_unmapped;
>>       NotifierWithReturn transfer_notifier;
>>       MemoryListener remap_listener;
>> -    int (*saved_dma_map)(const struct VFIOContainerBase *bcontainer,
>> -                         hwaddr iova, ram_addr_t size,
>> -                         void *vaddr, bool readonly, MemoryRegion *mr);
>> +    DMA_MAP_FUNC saved_dma_map;
>>   } VFIOContainerCPR;
>>   typedef struct VFIODeviceCPR {
>> diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
>> index a84c3247b7..100a8db74d 100644
>> --- a/hw/vfio/cpr-legacy.c
>> +++ b/hw/vfio/cpr-legacy.c
>> @@ -148,6 +148,7 @@ static int vfio_cpr_fail_notifier(NotifierWithReturn *notifier,
>>            */
>>           VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
>> +        DMA_MAP_FUNC saved_dma_map = vioc->dma_map;
>>           vioc->dma_map = vfio_legacy_cpr_dma_map;
>>           container->cpr.remap_listener = (MemoryListener) {
>> @@ -158,7 +159,7 @@ static int vfio_cpr_fail_notifier(NotifierWithReturn *notifier,
>>                                    bcontainer->space->as);
>>           memory_listener_unregister(&container->cpr.remap_listener);
>>           container->cpr.vaddr_unmapped = false;
>> -        vioc->dma_map = container->cpr.saved_dma_map;
>> +        vioc->dma_map = saved_dma_map;
>>       }
>>       return 0;
>>   }
>