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

2015-10-12 Thread Stefano Stabellini 
On Sun, 11 Oct 2015, Lan Tianyu wrote:
> From: >
> 
> msix->mmio is added to XenPCIPassthroughState's object as property.
> object_finalize_child_property is called for XenPCIPassthroughState's
> object, which calls object_property_del_all, which is going to try to
> delete msix->mmio. object_finalize_child_property() will access
> msix->mmio's obj. But the whole msix struct has already been freed
> by xen_pt_msix_delete. This will cause segment fault when msix->mmio
> has been overwritten.
> 
> This patch is to fix the issue.
> 
> Signed-off-by: Lan Tianyu 

Looks good to me. Paolo?


>  hw/xen/xen_pt.c |8 
>  hw/xen/xen_pt.h |1 +
>  hw/xen/xen_pt_config_init.c |2 +-
>  hw/xen/xen_pt_msi.c |   13 -
>  4 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 2b54f52..aa96288 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -938,10 +938,18 @@ static void xen_pci_passthrough_class_init(ObjectClass 
> *klass, void *data)
>  dc->props = xen_pci_passthrough_properties;
>  };
>  
> +static void xen_pci_passthrough_finalize(Object *obj)
> +{
> +XenPCIPassthroughState *s = XEN_PT_DEVICE(obj);
> +
> +xen_pt_msix_delete(s);
> +}
> +
>  static const TypeInfo xen_pci_passthrough_info = {
>  .name = TYPE_XEN_PT_DEVICE,
>  .parent = TYPE_PCI_DEVICE,
>  .instance_size = sizeof(XenPCIPassthroughState),
> +.instance_finalize = xen_pci_passthrough_finalize,
>  .class_init = xen_pci_passthrough_class_init,
>  };
>  
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 3bc22eb..c545280 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -305,6 +305,7 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s);
>  
>  int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base);
>  void xen_pt_msix_delete(XenPCIPassthroughState *s);
> +void xen_pt_msix_unmap(XenPCIPassthroughState *s);
>  int xen_pt_msix_update(XenPCIPassthroughState *s);
>  int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
>  void xen_pt_msix_disable(XenPCIPassthroughState *s);
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 4a5bc11..0efee11 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -2079,7 +2079,7 @@ void xen_pt_config_delete(XenPCIPassthroughState *s)
>  
>  /* free MSI/MSI-X info table */
>  if (s->msix) {
> -xen_pt_msix_delete(s);
> +xen_pt_msix_unmap(s);
>  }
>  g_free(s->msi);
>  
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index e3d7194..82de2bc 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(>bar[msix->bar_index], >mmio);
> +}
> +
> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> +{
> +XenPTMSIX *msix = s->msix;
> +
> +if (!msix) {
> +return;
> +}
> +
> +object_unparent(OBJECT(>mmio));
>  
>  g_free(s->msix);
>  s->msix = NULL;
> -- 
> 1.7.9.5
>

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

2015-10-12 Thread Stefano Stabellini 
On Mon, 12 Oct 2015, Paolo Bonzini wrote:
> On 12/10/2015 13:09, Stefano Stabellini wrote:
> > On Sun, 11 Oct 2015, Lan Tianyu wrote:
> >> From: >
> >>
> >> msix->mmio is added to XenPCIPassthroughState's object as property.
> >> object_finalize_child_property is called for XenPCIPassthroughState's
> >> object, which calls object_property_del_all, which is going to try to
> >> delete msix->mmio. object_finalize_child_property() will access
> >> msix->mmio's obj. But the whole msix struct has already been freed
> >> by xen_pt_msix_delete. This will cause segment fault when msix->mmio
> >> has been overwritten.
> >>
> >> This patch is to fix the issue.
> >>
> >> Signed-off-by: Lan Tianyu 
> > 
> > Looks good to me. Paolo?
> 
> Also looks good to me.  Thanks!

I'll add it to my tree.



> >>  hw/xen/xen_pt.c |8 
> >>  hw/xen/xen_pt.h |1 +
> >>  hw/xen/xen_pt_config_init.c |2 +-
> >>  hw/xen/xen_pt_msi.c |   13 -
> >>  4 files changed, 22 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> >> index 2b54f52..aa96288 100644
> >> --- a/hw/xen/xen_pt.c
> >> +++ b/hw/xen/xen_pt.c
> >> @@ -938,10 +938,18 @@ static void 
> >> xen_pci_passthrough_class_init(ObjectClass *klass, void *data)
> >>  dc->props = xen_pci_passthrough_properties;
> >>  };
> >>  
> >> +static void xen_pci_passthrough_finalize(Object *obj)
> >> +{
> >> +XenPCIPassthroughState *s = XEN_PT_DEVICE(obj);
> >> +
> >> +xen_pt_msix_delete(s);
> >> +}
> >> +
> >>  static const TypeInfo xen_pci_passthrough_info = {
> >>  .name = TYPE_XEN_PT_DEVICE,
> >>  .parent = TYPE_PCI_DEVICE,
> >>  .instance_size = sizeof(XenPCIPassthroughState),
> >> +.instance_finalize = xen_pci_passthrough_finalize,
> >>  .class_init = xen_pci_passthrough_class_init,
> >>  };
> >>  
> >> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> >> index 3bc22eb..c545280 100644
> >> --- a/hw/xen/xen_pt.h
> >> +++ b/hw/xen/xen_pt.h
> >> @@ -305,6 +305,7 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s);
> >>  
> >>  int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base);
> >>  void xen_pt_msix_delete(XenPCIPassthroughState *s);
> >> +void xen_pt_msix_unmap(XenPCIPassthroughState *s);
> >>  int xen_pt_msix_update(XenPCIPassthroughState *s);
> >>  int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
> >>  void xen_pt_msix_disable(XenPCIPassthroughState *s);
> >> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> >> index 4a5bc11..0efee11 100644
> >> --- a/hw/xen/xen_pt_config_init.c
> >> +++ b/hw/xen/xen_pt_config_init.c
> >> @@ -2079,7 +2079,7 @@ void xen_pt_config_delete(XenPCIPassthroughState *s)
> >>  
> >>  /* free MSI/MSI-X info table */
> >>  if (s->msix) {
> >> -xen_pt_msix_delete(s);
> >> +xen_pt_msix_unmap(s);
> >>  }
> >>  g_free(s->msi);
> >>  
> >> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> >> index e3d7194..82de2bc 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(>bar[msix->bar_index], >mmio);
> >> +}
> >> +
> >> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> >> +{
> >> +XenPTMSIX *msix = s->msix;
> >> +
> >> +if (!msix) {
> >> +return;
> >> +}
> >> +
> >> +object_unparent(OBJECT(>mmio));
> >>  
> >>  g_free(s->msix);
> >>  s->msix = NULL;
> >> -- 
> >> 1.7.9.5
> >>
>

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

2015-10-12 Thread Paolo Bonzini 


On 12/10/2015 13:09, Stefano Stabellini wrote:
> On Sun, 11 Oct 2015, Lan Tianyu wrote:
>> From: >
>>
>> msix->mmio is added to XenPCIPassthroughState's object as property.
>> object_finalize_child_property is called for XenPCIPassthroughState's
>> object, which calls object_property_del_all, which is going to try to
>> delete msix->mmio. object_finalize_child_property() will access
>> msix->mmio's obj. But the whole msix struct has already been freed
>> by xen_pt_msix_delete. This will cause segment fault when msix->mmio
>> has been overwritten.
>>
>> This patch is to fix the issue.
>>
>> Signed-off-by: Lan Tianyu 
> 
> Looks good to me. Paolo?

Also looks good to me.  Thanks!

Paolo

>>  hw/xen/xen_pt.c |8 
>>  hw/xen/xen_pt.h |1 +
>>  hw/xen/xen_pt_config_init.c |2 +-
>>  hw/xen/xen_pt_msi.c |   13 -
>>  4 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
>> index 2b54f52..aa96288 100644
>> --- a/hw/xen/xen_pt.c
>> +++ b/hw/xen/xen_pt.c
>> @@ -938,10 +938,18 @@ static void xen_pci_passthrough_class_init(ObjectClass 
>> *klass, void *data)
>>  dc->props = xen_pci_passthrough_properties;
>>  };
>>  
>> +static void xen_pci_passthrough_finalize(Object *obj)
>> +{
>> +XenPCIPassthroughState *s = XEN_PT_DEVICE(obj);
>> +
>> +xen_pt_msix_delete(s);
>> +}
>> +
>>  static const TypeInfo xen_pci_passthrough_info = {
>>  .name = TYPE_XEN_PT_DEVICE,
>>  .parent = TYPE_PCI_DEVICE,
>>  .instance_size = sizeof(XenPCIPassthroughState),
>> +.instance_finalize = xen_pci_passthrough_finalize,
>>  .class_init = xen_pci_passthrough_class_init,
>>  };
>>  
>> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
>> index 3bc22eb..c545280 100644
>> --- a/hw/xen/xen_pt.h
>> +++ b/hw/xen/xen_pt.h
>> @@ -305,6 +305,7 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s);
>>  
>>  int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base);
>>  void xen_pt_msix_delete(XenPCIPassthroughState *s);
>> +void xen_pt_msix_unmap(XenPCIPassthroughState *s);
>>  int xen_pt_msix_update(XenPCIPassthroughState *s);
>>  int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
>>  void xen_pt_msix_disable(XenPCIPassthroughState *s);
>> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
>> index 4a5bc11..0efee11 100644
>> --- a/hw/xen/xen_pt_config_init.c
>> +++ b/hw/xen/xen_pt_config_init.c
>> @@ -2079,7 +2079,7 @@ void xen_pt_config_delete(XenPCIPassthroughState *s)
>>  
>>  /* free MSI/MSI-X info table */
>>  if (s->msix) {
>> -xen_pt_msix_delete(s);
>> +xen_pt_msix_unmap(s);
>>  }
>>  g_free(s->msi);
>>  
>> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
>> index e3d7194..82de2bc 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(>bar[msix->bar_index], >mmio);
>> +}
>> +
>> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
>> +{
>> +XenPTMSIX *msix = s->msix;
>> +
>> +if (!msix) {
>> +return;
>> +}
>> +
>> +object_unparent(OBJECT(>mmio));
>>  
>>  g_free(s->msix);
>>  s->msix = NULL;
>> -- 
>> 1.7.9.5
>>