| Message ID | 1444576764-15344-1-git-send-email-tianyu.lan@intel.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >>
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 > >> >
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;