* [Qemu-devel] [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
@ 2015-10-11 15:19 ` Lan Tianyu
0 siblings, 0 replies; 8+ messages in thread
From: Lan Tianyu @ 2015-10-11 15:19 UTC (permalink / raw)
To: stefano.stabellini, pbonzini, mjt; +Cc: Lan Tianyu, xen-devel, qemu-devel
From: <tianyu.lan@intel.com>>
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 <tianyu.lan@intel.com>
---
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(&s->bar[msix->bar_index], &msix->mmio);
+}
+
+void xen_pt_msix_delete(XenPCIPassthroughState *s)
+{
+ XenPTMSIX *msix = s->msix;
+
+ if (!msix) {
+ return;
+ }
+
+ object_unparent(OBJECT(&msix->mmio));
g_free(s->msix);
s->msix = NULL;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
@ 2015-10-11 15:19 ` Lan Tianyu
0 siblings, 0 replies; 8+ messages in thread
From: Lan Tianyu @ 2015-10-11 15:19 UTC (permalink / raw)
To: stefano.stabellini, pbonzini, mjt; +Cc: Lan Tianyu, xen-devel, qemu-devel
From: <tianyu.lan@intel.com>>
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 <tianyu.lan@intel.com>
---
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(&s->bar[msix->bar_index], &msix->mmio);
+}
+
+void xen_pt_msix_delete(XenPCIPassthroughState *s)
+{
+ XenPTMSIX *msix = s->msix;
+
+ if (!msix) {
+ return;
+ }
+
+ object_unparent(OBJECT(&msix->mmio));
g_free(s->msix);
s->msix = NULL;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
2015-10-11 15:19 ` Lan Tianyu
@ 2015-10-12 11:09 ` Stefano Stabellini
-1 siblings, 0 replies; 8+ messages in thread
From: Stefano Stabellini @ 2015-10-12 11:09 UTC (permalink / raw)
To: Lan Tianyu; +Cc: pbonzini, xen-devel, mjt, qemu-devel, stefano.stabellini
On Sun, 11 Oct 2015, Lan Tianyu wrote:
> From: <tianyu.lan@intel.com>>
>
> 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 <tianyu.lan@intel.com>
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(&s->bar[msix->bar_index], &msix->mmio);
> +}
> +
> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> +{
> + XenPTMSIX *msix = s->msix;
> +
> + if (!msix) {
> + return;
> + }
> +
> + object_unparent(OBJECT(&msix->mmio));
>
> g_free(s->msix);
> s->msix = NULL;
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
@ 2015-10-12 11:09 ` Stefano Stabellini
0 siblings, 0 replies; 8+ messages in thread
From: Stefano Stabellini @ 2015-10-12 11:09 UTC (permalink / raw)
To: Lan Tianyu; +Cc: pbonzini, xen-devel, mjt, qemu-devel, stefano.stabellini
On Sun, 11 Oct 2015, Lan Tianyu wrote:
> From: <tianyu.lan@intel.com>>
>
> 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 <tianyu.lan@intel.com>
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(&s->bar[msix->bar_index], &msix->mmio);
> +}
> +
> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> +{
> + XenPTMSIX *msix = s->msix;
> +
> + if (!msix) {
> + return;
> + }
> +
> + object_unparent(OBJECT(&msix->mmio));
>
> g_free(s->msix);
> s->msix = NULL;
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
2015-10-12 11:09 ` Stefano Stabellini
@ 2015-10-12 12:01 ` Paolo Bonzini
-1 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2015-10-12 12:01 UTC (permalink / raw)
To: Stefano Stabellini, Lan Tianyu; +Cc: xen-devel, mjt, qemu-devel
On 12/10/2015 13:09, Stefano Stabellini wrote:
> On Sun, 11 Oct 2015, Lan Tianyu wrote:
>> From: <tianyu.lan@intel.com>>
>>
>> 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 <tianyu.lan@intel.com>
>
> 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(&s->bar[msix->bar_index], &msix->mmio);
>> +}
>> +
>> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
>> +{
>> + XenPTMSIX *msix = s->msix;
>> +
>> + if (!msix) {
>> + return;
>> + }
>> +
>> + object_unparent(OBJECT(&msix->mmio));
>>
>> g_free(s->msix);
>> s->msix = NULL;
>> --
>> 1.7.9.5
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
@ 2015-10-12 12:01 ` Paolo Bonzini
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2015-10-12 12:01 UTC (permalink / raw)
To: Stefano Stabellini, Lan Tianyu; +Cc: xen-devel, mjt, qemu-devel
On 12/10/2015 13:09, Stefano Stabellini wrote:
> On Sun, 11 Oct 2015, Lan Tianyu wrote:
>> From: <tianyu.lan@intel.com>>
>>
>> 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 <tianyu.lan@intel.com>
>
> 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(&s->bar[msix->bar_index], &msix->mmio);
>> +}
>> +
>> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
>> +{
>> + XenPTMSIX *msix = s->msix;
>> +
>> + if (!msix) {
>> + return;
>> + }
>> +
>> + object_unparent(OBJECT(&msix->mmio));
>>
>> g_free(s->msix);
>> s->msix = NULL;
>> --
>> 1.7.9.5
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
2015-10-12 12:01 ` Paolo Bonzini
@ 2015-10-12 12:45 ` Stefano Stabellini
-1 siblings, 0 replies; 8+ messages in thread
From: Stefano Stabellini @ 2015-10-12 12:45 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Lan Tianyu, xen-devel, mjt, qemu-devel, 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: <tianyu.lan@intel.com>>
> >>
> >> 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 <tianyu.lan@intel.com>
> >
> > 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(&s->bar[msix->bar_index], &msix->mmio);
> >> +}
> >> +
> >> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> >> +{
> >> + XenPTMSIX *msix = s->msix;
> >> +
> >> + if (!msix) {
> >> + return;
> >> + }
> >> +
> >> + object_unparent(OBJECT(&msix->mmio));
> >>
> >> g_free(s->msix);
> >> s->msix = NULL;
> >> --
> >> 1.7.9.5
> >>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region
@ 2015-10-12 12:45 ` Stefano Stabellini
0 siblings, 0 replies; 8+ messages in thread
From: Stefano Stabellini @ 2015-10-12 12:45 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Lan Tianyu, xen-devel, mjt, qemu-devel, 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: <tianyu.lan@intel.com>>
> >>
> >> 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 <tianyu.lan@intel.com>
> >
> > 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(&s->bar[msix->bar_index], &msix->mmio);
> >> +}
> >> +
> >> +void xen_pt_msix_delete(XenPCIPassthroughState *s)
> >> +{
> >> + XenPTMSIX *msix = s->msix;
> >> +
> >> + if (!msix) {
> >> + return;
> >> + }
> >> +
> >> + object_unparent(OBJECT(&msix->mmio));
> >>
> >> g_free(s->msix);
> >> s->msix = NULL;
> >> --
> >> 1.7.9.5
> >>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-10-12 12:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-11 15:19 [Qemu-devel] [PATCH] Qemu/Xen: Fix early freeing MSIX MMIO memory region Lan Tianyu
2015-10-11 15:19 ` Lan Tianyu
2015-10-12 11:09 ` [Qemu-devel] " Stefano Stabellini
2015-10-12 11:09 ` Stefano Stabellini
2015-10-12 12:01 ` [Qemu-devel] " Paolo Bonzini
2015-10-12 12:01 ` Paolo Bonzini
2015-10-12 12:45 ` [Qemu-devel] " Stefano Stabellini
2015-10-12 12:45 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.