[Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region

[Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region

[Lan Tianyu]

MSIX MMIO memory region is added to pt device's obj as property.
When pt device is unplugged, all properties will be deleted and
memory region's obj is needed at that point(refer object_finalize_child_property()).
But current code frees MSIX MMIO memory region in the xen_pt_msix_delete()
before deleting pt device's properties, this will cause segment fault.
Reproduce the bug via hotplugging device frequently.

This patch is to fix the issue via moving MSIX MMIO memory region into
struct XenPCIPassthroughState and free it together with pt device's obj.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 hw/xen/xen_pt.c     |    4 ++--
 hw/xen/xen_pt.h     |    2 +-
 hw/xen/xen_pt_msi.c |    6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 2b54f52..0c11069 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -587,11 +587,11 @@ static void xen_pt_region_update(XenPCIPassthroughState *s,
     };
 
     bar = xen_pt_bar_from_region(s, mr);
-    if (bar == -1 && (!s->msix || &s->msix->mmio != mr)) {
+    if (bar == -1 && (!s->msix || &s->msix_mmio != mr)) {
         return;
     }
 
-    if (s->msix && &s->msix->mmio == mr) {
+    if (s->msix && &s->msix_mmio == mr) {
         if (adding) {
             s->msix->mmio_base_addr = sec->offset_within_address_space;
             rc = xen_pt_msix_update_remap(s, s->msix->bar_index);
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 3bc22eb..3569c2c 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -199,7 +199,6 @@ typedef struct XenPTMSIX {
     uint64_t table_base;
     uint32_t table_offset_adjust; /* page align mmap */
     uint64_t mmio_base_addr;
-    MemoryRegion mmio;
     void *phys_iomem_base;
     XenPTMSIXEntry msix_entry[0];
 } XenPTMSIX;
@@ -222,6 +221,7 @@ struct XenPCIPassthroughState {
 
     MemoryRegion bar[PCI_NUM_REGIONS - 1];
     MemoryRegion rom;
+    MemoryRegion msix_mmio;
 
     MemoryListener memory_listener;
     MemoryListener io_listener;
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index e3d7194..ae39ab3 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -558,7 +558,7 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
         msix->msix_entry[i].pirq = XEN_PT_UNASSIGNED_PIRQ;
     }
 
-    memory_region_init_io(&msix->mmio, OBJECT(s), &pci_msix_ops,
+    memory_region_init_io(&s->msix_mmio, OBJECT(s), &pci_msix_ops,
                           s, "xen-pci-pt-msix",
                           (total_entries * PCI_MSIX_ENTRY_SIZE
                            + XC_PAGE_SIZE - 1)
@@ -599,7 +599,7 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
                msix->phys_iomem_base);
 
     memory_region_add_subregion_overlap(&s->bar[bar_index], table_off,
-                                        &msix->mmio,
+                                        &s->msix_mmio,
                                         2); /* Priority: pci default + 1 */
 
     return 0;
@@ -626,7 +626,7 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
                + msix->table_offset_adjust);
     }
 
-    memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
+    memory_region_del_subregion(&s->bar[msix->bar_index], &s->msix_mmio);
 
     g_free(s->msix);
     s->msix = NULL;
-- 
1.7.9.5

Re: [Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region

[Stefano Stabellini]

Hi Lan, thanks for the patch.

On Wed, 30 Sep 2015, Lan Tianyu wrote:
> MSIX MMIO memory region is added to pt device's obj as property.

msix->mmio is added to XenPCIPassthroughState's object as property


> When pt device is unplugged, all properties will be deleted and
> memory region's obj is needed at that point(refer object_finalize_child_property()).

object_finalize_child_property is called for XenPCIPassthroughState's
object, which calls object_property_del_all, which is going to try to
delete (again) msix->mmio. But the whole msix struct could have already
been freed by xen_pt_msix_delete.


> But current code frees MSIX MMIO memory region in the xen_pt_msix_delete()
> before deleting pt device's properties, this will cause segment fault.
> Reproduce the bug via hotplugging device frequently.

What is the line of code under object_property_del_all that actually
causes the seg fault?


> This patch is to fix the issue via moving MSIX MMIO memory region into
> struct XenPCIPassthroughState and free it together with pt device's obj.

Given that all the MSI-X related info are in XenPTMSIX, I would prefer
to keep the mmio memory region there, if possible.

Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
in xen_pt_msix_delete?  Calling object_property_del_child or
object_unparent?


> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>
>  hw/xen/xen_pt.c     |    4 ++--
>  hw/xen/xen_pt.h     |    2 +-
>  hw/xen/xen_pt_msi.c |    6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 2b54f52..0c11069 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -587,11 +587,11 @@ static void xen_pt_region_update(XenPCIPassthroughState *s,
>      };
>  
>      bar = xen_pt_bar_from_region(s, mr);
> -    if (bar == -1 && (!s->msix || &s->msix->mmio != mr)) {
> +    if (bar == -1 && (!s->msix || &s->msix_mmio != mr)) {
>          return;
>      }
>  
> -    if (s->msix && &s->msix->mmio == mr) {
> +    if (s->msix && &s->msix_mmio == mr) {
>          if (adding) {
>              s->msix->mmio_base_addr = sec->offset_within_address_space;
>              rc = xen_pt_msix_update_remap(s, s->msix->bar_index);
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 3bc22eb..3569c2c 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -199,7 +199,6 @@ typedef struct XenPTMSIX {
>      uint64_t table_base;
>      uint32_t table_offset_adjust; /* page align mmap */
>      uint64_t mmio_base_addr;
> -    MemoryRegion mmio;
>      void *phys_iomem_base;
>      XenPTMSIXEntry msix_entry[0];
>  } XenPTMSIX;
> @@ -222,6 +221,7 @@ struct XenPCIPassthroughState {
>  
>      MemoryRegion bar[PCI_NUM_REGIONS - 1];
>      MemoryRegion rom;
> +    MemoryRegion msix_mmio;
>  
>      MemoryListener memory_listener;
>      MemoryListener io_listener;
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index e3d7194..ae39ab3 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -558,7 +558,7 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
>          msix->msix_entry[i].pirq = XEN_PT_UNASSIGNED_PIRQ;
>      }
>  
> -    memory_region_init_io(&msix->mmio, OBJECT(s), &pci_msix_ops,
> +    memory_region_init_io(&s->msix_mmio, OBJECT(s), &pci_msix_ops,
>                            s, "xen-pci-pt-msix",
>                            (total_entries * PCI_MSIX_ENTRY_SIZE
>                             + XC_PAGE_SIZE - 1)
> @@ -599,7 +599,7 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
>                 msix->phys_iomem_base);
>  
>      memory_region_add_subregion_overlap(&s->bar[bar_index], table_off,
> -                                        &msix->mmio,
> +                                        &s->msix_mmio,
>                                          2); /* Priority: pci default + 1 */
>  
>      return 0;
> @@ -626,7 +626,7 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
>                 + msix->table_offset_adjust);
>      }
>  
> -    memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
> +    memory_region_del_subregion(&s->bar[msix->bar_index], &s->msix_mmio);
>  
>      g_free(s->msix);
>      s->msix = NULL;
> -- 
> 1.7.9.5
> 

Re: [Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region

[Paolo Bonzini]

On 05/10/2015 18:53, Stefano Stabellini wrote:
>> This patch is to fix the issue via moving MSIX MMIO memory region into
>> struct XenPCIPassthroughState and free it together with pt device's obj.
> 
> Given that all the MSI-X related info are in XenPTMSIX, I would prefer
> to keep the mmio memory region there, if possible.
> 
> Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
> in xen_pt_msix_delete?  Calling object_property_del_child or
> object_unparent?

This is the right thing to do, but there are two separate things to fix.

One is the use-after-free of msix->mmio, the other is that freeing 
s->msix and in general xen_pt_config_delete should be done from the 
.instance_finalize callback.  This is documented in docs/memory.txt.

This is an attempt at a patch (not even compiled):

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index e3d7194..e476bac 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -610,7 +610,7 @@ error_out:
     return rc;
 }
 
-void xen_pt_msix_delete(XenPCIPassthroughState *s)
+void xen_pt_msix_unmap(XenPCIPassthroughState *s)
 {
     XenPTMSIX *msix = s->msix;
 
@@ -627,6 +627,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
     }
 
     memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
+}
+
+void xen_pt_msix_delete(XenPCIPassthroughState *s)
+{
+    XenPTMSIX *msix = s->msix;
+
+    if (!msix) {
+        return;
+    }
+
+    object_unparent(&msix->mmio);
 
     g_free(s->msix);
     s->msix = NULL;

where xen_pt_config_unmap would be called from xen_pt_destroy, and the call
to xen_pt_config_delete would be moved to xen_pci_passthrough_info's
instance_finalize member.

Paolo

Re: [Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region

[Stefano Stabellini]

On Tue, 6 Oct 2015, Paolo Bonzini wrote:
> On 05/10/2015 18:53, Stefano Stabellini wrote:
> >> This patch is to fix the issue via moving MSIX MMIO memory region into
> >> struct XenPCIPassthroughState and free it together with pt device's obj.
> > 
> > Given that all the MSI-X related info are in XenPTMSIX, I would prefer
> > to keep the mmio memory region there, if possible.
> > 
> > Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
> > in xen_pt_msix_delete?  Calling object_property_del_child or
> > object_unparent?
> 
> This is the right thing to do, but there are two separate things to fix.
> 
> One is the use-after-free of msix->mmio, the other is that freeing 
> s->msix and in general xen_pt_config_delete should be done from the 
> .instance_finalize callback.  This is documented in docs/memory.txt.
> 
> This is an attempt at a patch (not even compiled):
> 
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index e3d7194..e476bac 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -610,7 +610,7 @@ error_out:
>      return rc;
>  }
>  
> -void xen_pt_msix_delete(XenPCIPassthroughState *s)
> +void xen_pt_msix_unmap(XenPCIPassthroughState *s)
>  {
>      XenPTMSIX *msix = s->msix;
>  
> @@ -627,6 +627,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
>      }
>  
>      memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
> +}
> +
> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> +{
> +    XenPTMSIX *msix = s->msix;
> +
> +    if (!msix) {
> +        return;
> +    }
> +
> +    object_unparent(&msix->mmio);
>  
>      g_free(s->msix);
>      s->msix = NULL;
> 
> where xen_pt_config_unmap would be called from xen_pt_destroy, and the call
> to xen_pt_config_delete would be moved to xen_pci_passthrough_info's
> instance_finalize member.

Thanks for the explanation and the code. It makes sense to me.

Lan, could you please write up a patch based on this approach and test
it?

Re: [Qemu-devel] [Fix PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region

[Lan, Tianyu]

On 10/6/2015 9:49 PM, Stefano Stabellini wrote:
> On Tue, 6 Oct 2015, Paolo Bonzini wrote:
>> On 05/10/2015 18:53, Stefano Stabellini wrote:
>>>> This patch is to fix the issue via moving MSIX MMIO memory region into
>>>> struct XenPCIPassthroughState and free it together with pt device's obj.
>>>
>>> Given that all the MSI-X related info are in XenPTMSIX, I would prefer
>>> to keep the mmio memory region there, if possible.
>>>
>>> Couldn't you just unhook msix->mmio from XenPCIPassthroughState's object
>>> in xen_pt_msix_delete?  Calling object_property_del_child or
>>> object_unparent?
>>
>> This is the right thing to do, but there are two separate things to fix.
>>
>> One is the use-after-free of msix->mmio, the other is that freeing
>> s->msix and in general xen_pt_config_delete should be done from the
>> .instance_finalize callback.  This is documented in docs/memory.txt.
>>
>> This is an attempt at a patch (not even compiled):
>>
>> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
>> index e3d7194..e476bac 100644
>> --- a/hw/xen/xen_pt_msi.c
>> +++ b/hw/xen/xen_pt_msi.c
>> @@ -610,7 +610,7 @@ error_out:
>>       return rc;
>>   }
>>
>> -void xen_pt_msix_delete(XenPCIPassthroughState *s)
>> +void xen_pt_msix_unmap(XenPCIPassthroughState *s)
>>   {
>>       XenPTMSIX *msix = s->msix;
>>
>> @@ -627,6 +627,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
>>       }
>>
>>       memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
>> +}
>> +
>> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
>> +{
>> +    XenPTMSIX *msix = s->msix;
>> +
>> +    if (!msix) {
>> +        return;
>> +    }
>> +
>> +    object_unparent(&msix->mmio);
>>
>>       g_free(s->msix);
>>       s->msix = NULL;
>>
>> where xen_pt_config_unmap would be called from xen_pt_destroy, and the call
>> to xen_pt_config_delete would be moved to xen_pci_passthrough_info's
>> instance_finalize member.
>
> Thanks for the explanation and the code. It makes sense to me.
>
> Lan, could you please write up a patch based on this approach and test
> it?
>

Sure. I will update patch following the guide. Thanks Paolo & Stefano.