[v1] Some trivial live update fixes

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

Posted by Zhenzhong Duan 4 months, 3 weeks ago

cpr.saved_dma_map isn't initialized in source qemu which lead to vioc->dma_map
assigned a NULL value, this will trigger SIGSEGV.

Fix it by save and restore vioc->dma_map locally.

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

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

Posted by Cédric Le Goater 4 months, 3 weeks ago

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 ?


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

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

Posted by Steven Sistare 4 months, 3 weeks ago

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

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

Posted by Steven Sistare 4 months, 3 weeks ago

On 6/26/2025 8:53 AM, Steven Sistare wrote:
> 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;
>       }

Now I see that patch 4 deletes saved_dma_map entirely, and looks fine.
I still think the above diff is the best fix for patch 3, and you
can move the "save and restore locally" parts of patch 3 to patch 4.

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

[PATCH 1/4] vfio/container: Fix SIGSEGV when open container file fails
[PATCH 2/4] vfio/container: fails mdev hotplug if add migration blocker failed
[PATCH 3/4] vfio/container: Fix potential SIGSEGV when recover from unmap-all-vaddr failure
[PATCH 4/4] vfio/container: Fix vfio_container_post_load()