* [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability @ 2014-06-25 9:59 SeokYeon Hwang 2014-06-25 12:03 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: SeokYeon Hwang @ 2014-06-25 9:59 UTC (permalink / raw) To: qemu-devel, mst; +Cc: syeon.hwang In case of the unrealized "pdev", memory can be illegally accessed and corrupted. Refer to device_unparent() in the commit 5c21ce77d7e5643089ceec556c0408445d017f32. Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> --- hw/pci/pci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 49eca95..bb7f0c5 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, /* Unlink capability from the pci config space. */ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) { - uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, &prev); + uint8_t prev, offset; + /* Check whether the device is realized or not */ + if (!pdev->qdev.realized) { + return; + } + offset = pci_find_capability_list(pdev, cap_id, &prev); if (!offset) return; pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT]; -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability 2014-06-25 9:59 [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability SeokYeon Hwang @ 2014-06-25 12:03 ` Michael S. Tsirkin 2014-06-25 12:28 ` Marcel Apfelbaum 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2014-06-25 12:03 UTC (permalink / raw) To: SeokYeon Hwang; +Cc: qemu-devel On Wed, Jun 25, 2014 at 06:59:08PM +0900, SeokYeon Hwang wrote: > In case of the unrealized "pdev", memory can be illegally accessed and corrupted. > Refer to device_unparent() in the commit 5c21ce77d7e5643089ceec556c0408445d017f32. > > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> Another case of qemu mailing list dropping patches :( I will bounce it now so people can see the original message. > --- > hw/pci/pci.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 49eca95..bb7f0c5 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, > /* Unlink capability from the pci config space. */ > void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) > { > - uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, &prev); > + uint8_t prev, offset; > + /* Check whether the device is realized or not */ > + if (!pdev->qdev.realized) { > + return; > + } > + offset = pci_find_capability_list(pdev, cap_id, &prev); > if (!offset) > return; > pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT]; > -- > 1.9.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability 2014-06-25 12:03 ` Michael S. Tsirkin @ 2014-06-25 12:28 ` Marcel Apfelbaum 2014-06-26 3:48 ` SeokYeon Hwang 0 siblings, 1 reply; 6+ messages in thread From: Marcel Apfelbaum @ 2014-06-25 12:28 UTC (permalink / raw) To: SeokYeon Hwang; +Cc: qemu-devel@nongnu.org, Michael S. Tsirkin On Wed, 2014-06-25 at 15:03 +0300, Michael S. Tsirkin wrote: > On Wed, Jun 25, 2014 at 06:59:08PM +0900, SeokYeon Hwang wrote: > > In case of the unrealized "pdev", memory can be illegally accessed and corrupted. > > Refer to device_unparent() in the commit 5c21ce77d7e5643089ceec556c0408445d017f32. Hi, Thank you for submitting the patch. Can you please send to the list how to reproduce the issue? Is this a regression? Before the above commit did it work? > > > > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae > > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> > > Another case of qemu mailing list dropping patches :( > I will bounce it now so people can see the original > message. > > > > --- > > hw/pci/pci.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 49eca95..bb7f0c5 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, > > /* Unlink capability from the pci config space. */ > > void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) > > { > > - uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, &prev); > > + uint8_t prev, offset; > > + /* Check whether the device is realized or not */ > > + if (!pdev->qdev.realized) { The 'qdev' field and 'realize' property are private, you should use one of QOM's 'property_get' methods. I am also concerned about adding the realize check here, it seems too "deep" in implementation, seeing this checks everywhere doesn't seem a good idea. I think we should first understand the root cause and try making this change in a higher level. I hope I helped, Marcel > > + return; > > + } > > + offset = pci_find_capability_list(pdev, cap_id, &prev); > > if (!offset) > > return; > > pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT]; > > -- > > 1.9.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability 2014-06-25 12:28 ` Marcel Apfelbaum @ 2014-06-26 3:48 ` SeokYeon Hwang 2014-06-26 7:07 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: SeokYeon Hwang @ 2014-06-26 3:48 UTC (permalink / raw) To: marcel.a; +Cc: qemu-devel, 'Michael S. Tsirkin' > On Wed, 2014-06-25 at 15:03 +0300, Michael S. Tsirkin wrote: > > On Wed, Jun 25, 2014 at 06:59:08PM +0900, SeokYeon Hwang wrote: > > > In case of the unrealized "pdev", memory can be illegally accessed and > corrupted. > > > Refer to device_unparent() in the commit > 5c21ce77d7e5643089ceec556c0408445d017f32. > Hi, > Thank you for submitting the patch. > > Can you please send to the list how to reproduce the issue? > Is this a regression? Before the above commit did it work? Hi, When VirtIO device is hot-unplugged, Qemu unrealizes the device first then child_bus by device_unparent(). (After the commit 5c21ce77d7e5643089ceec556c0408445d017f32.) For example, if I suppose to hot-unplug "virtio block" device, unparent "virtio-blk-pci" first, then "virtio-blk". virtio_bus_device_unplugged() function in virtio-bus.c calls "msix_uninit()" with the parameter of its parent PCI device. The problem is that it accesses already freed member of pdev, such as "pdev->config". This illegal access causes the heap corruption, and it kills Qemu process suddenly. You can detect this illegal access by using "electric fence" or "valgrind". > > > > > > > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae > > > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> > > > > Another case of qemu mailing list dropping patches :( I will bounce it > > now so people can see the original message. > > > > > > > --- > > > hw/pci/pci.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 49eca95..bb7f0c5 > > > 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev, > > > uint8_t cap_id, > > > /* Unlink capability from the pci config space. */ void > > > pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) { > > > - uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, > &prev); > > > + uint8_t prev, offset; > > > + /* Check whether the device is realized or not */ > > > + if (!pdev->qdev.realized) { > The 'qdev' field and 'realize' property are private, you should use one of > QOM's 'property_get' methods. > > I am also concerned about adding the realize check here, it seems too > "deep" in implementation, seeing this checks everywhere doesn't seem a > good idea. > I think we should first understand the root cause and try making this > change in a higher level. > > I hope I helped, > Marcel Ok, I see. I think you're right. If needed, I will send an another patch using "object_property_get_bool()". Thanks. > > > > > + return; > > > + } > > > + offset = pci_find_capability_list(pdev, cap_id, &prev); > > > if (!offset) > > > return; > > > pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT]; > > > -- > > > 1.9.1 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability 2014-06-26 3:48 ` SeokYeon Hwang @ 2014-06-26 7:07 ` Michael S. Tsirkin 2014-06-28 6:39 ` SeokYeon Hwang 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2014-06-26 7:07 UTC (permalink / raw) To: SeokYeon Hwang; +Cc: qemu-devel, marcel.a On Thu, Jun 26, 2014 at 12:48:14PM +0900, SeokYeon Hwang wrote: > > On Wed, 2014-06-25 at 15:03 +0300, Michael S. Tsirkin wrote: > > > On Wed, Jun 25, 2014 at 06:59:08PM +0900, SeokYeon Hwang wrote: > > > > In case of the unrealized "pdev", memory can be illegally accessed and > > corrupted. > > > > Refer to device_unparent() in the commit > > 5c21ce77d7e5643089ceec556c0408445d017f32. > > Hi, > > Thank you for submitting the patch. > > > > Can you please send to the list how to reproduce the issue? > > Is this a regression? Before the above commit did it work? > > Hi, > > When VirtIO device is hot-unplugged, Qemu unrealizes the device first then child_bus by device_unparent(). > (After the commit 5c21ce77d7e5643089ceec556c0408445d017f32.) > > For example, if I suppose to hot-unplug "virtio block" device, unparent "virtio-blk-pci" first, then "virtio-blk". > > virtio_bus_device_unplugged() function in virtio-bus.c calls "msix_uninit()" with the parameter of its parent PCI device. > The problem is that it accesses already freed member of pdev, such as "pdev->config". > This illegal access causes the heap corruption, and it kills Qemu process suddenly. > > You can detect this illegal access by using "electric fence" or "valgrind". Does this still trigger for you with latest master? I tried this: diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 17ed510..fc81803 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -791,6 +791,11 @@ static void pci_config_free(PCIDevice *pci_dev) g_free(pci_dev->wmask); g_free(pci_dev->w1cmask); g_free(pci_dev->used); + pci_dev->config = NULL; + pci_dev->cmask = NULL; + pci_dev->wmask = NULL; + pci_dev->w1cmask = NULL; + pci_dev->used = NULL; } static void do_pci_unregister_device(PCIDevice *pci_dev) does not trigger crash after hot-unplug of virtio-net-pci on master. > > > > > > > > > > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae > > > > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> > > > > > > Another case of qemu mailing list dropping patches :( I will bounce it > > > now so people can see the original message. > > > > > > > > > > --- > > > > hw/pci/pci.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 49eca95..bb7f0c5 > > > > 100644 > > > > --- a/hw/pci/pci.c > > > > +++ b/hw/pci/pci.c > > > > @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev, > > > > uint8_t cap_id, > > > > /* Unlink capability from the pci config space. */ void > > > > pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) { > > > > - uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, > > &prev); > > > > + uint8_t prev, offset; > > > > + /* Check whether the device is realized or not */ > > > > + if (!pdev->qdev.realized) { > > The 'qdev' field and 'realize' property are private, you should use one of > > QOM's 'property_get' methods. > > > > I am also concerned about adding the realize check here, it seems too > > "deep" in implementation, seeing this checks everywhere doesn't seem a > > good idea. > > I think we should first understand the root cause and try making this > > change in a higher level. > > > > I hope I helped, > > Marcel > > Ok, I see. I think you're right. > If needed, I will send an another patch using "object_property_get_bool()". > > Thanks. > > > > > > > > > + return; > > > > + } > > > > + offset = pci_find_capability_list(pdev, cap_id, &prev); > > > > if (!offset) > > > > return; > > > > pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT]; > > > > -- > > > > 1.9.1 > > > > > > ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability 2014-06-26 7:07 ` Michael S. Tsirkin @ 2014-06-28 6:39 ` SeokYeon Hwang 0 siblings, 0 replies; 6+ messages in thread From: SeokYeon Hwang @ 2014-06-28 6:39 UTC (permalink / raw) To: 'Michael S. Tsirkin'; +Cc: syeon.hwang, qemu-devel, marcel.a When I check it the latest master branch, the issue is already resolved. It is resolved by commit 5942a19040fed313b316ab7b6e3d2d8e7b1625bb (28, June). I'm sorry and thank you for your help. > On Thu, Jun 26, 2014 at 12:48:14PM +0900, SeokYeon Hwang wrote: > > > On Wed, 2014-06-25 at 15:03 +0300, Michael S. Tsirkin wrote: > > > > On Wed, Jun 25, 2014 at 06:59:08PM +0900, SeokYeon Hwang wrote: > > > > > In case of the unrealized "pdev", memory can be illegally > > > > > accessed and > > > corrupted. > > > > > Refer to device_unparent() in the commit > > > 5c21ce77d7e5643089ceec556c0408445d017f32. > > > Hi, > > > Thank you for submitting the patch. > > > > > > Can you please send to the list how to reproduce the issue? > > > Is this a regression? Before the above commit did it work? > > > > Hi, > > > > When VirtIO device is hot-unplugged, Qemu unrealizes the device first > then child_bus by device_unparent(). > > (After the commit 5c21ce77d7e5643089ceec556c0408445d017f32.) > > > > For example, if I suppose to hot-unplug "virtio block" device, unparent > "virtio-blk-pci" first, then "virtio-blk". > > > > virtio_bus_device_unplugged() function in virtio-bus.c calls > "msix_uninit()" with the parameter of its parent PCI device. > > The problem is that it accesses already freed member of pdev, such as > "pdev->config". > > This illegal access causes the heap corruption, and it kills Qemu > process suddenly. > > > > You can detect this illegal access by using "electric fence" or > "valgrind". > > Does this still trigger for you with latest master? > I tried this: > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 17ed510..fc81803 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -791,6 +791,11 @@ static void pci_config_free(PCIDevice *pci_dev) > g_free(pci_dev->wmask); > g_free(pci_dev->w1cmask); > g_free(pci_dev->used); > + pci_dev->config = NULL; > + pci_dev->cmask = NULL; > + pci_dev->wmask = NULL; > + pci_dev->w1cmask = NULL; > + pci_dev->used = NULL; > } > > static void do_pci_unregister_device(PCIDevice *pci_dev) > > > does not trigger crash after hot-unplug of virtio-net-pci on master. > > > > > > > > > > > > > > > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae > > > > > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com> > > > > > > > > Another case of qemu mailing list dropping patches :( I will > > > > bounce it now so people can see the original message. > > > > > > > > > > > > > --- > > > > > hw/pci/pci.c | 7 ++++++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 49eca95..bb7f0c5 > > > > > 100644 > > > > > --- a/hw/pci/pci.c > > > > > +++ b/hw/pci/pci.c > > > > > @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev, > > > > > uint8_t cap_id, > > > > > /* Unlink capability from the pci config space. */ void > > > > > pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) > { > > > > > - uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, > > > &prev); > > > > > + uint8_t prev, offset; > > > > > + /* Check whether the device is realized or not */ > > > > > + if (!pdev->qdev.realized) { > > > The 'qdev' field and 'realize' property are private, you should use > > > one of QOM's 'property_get' methods. > > > > > > I am also concerned about adding the realize check here, it seems > > > too "deep" in implementation, seeing this checks everywhere doesn't > > > seem a good idea. > > > I think we should first understand the root cause and try making > > > this change in a higher level. > > > > > > I hope I helped, > > > Marcel > > > > Ok, I see. I think you're right. > > If needed, I will send an another patch using > "object_property_get_bool()". > > > > Thanks. > > > > > > > > > > > > > + return; > > > > > + } > > > > > + offset = pci_find_capability_list(pdev, cap_id, &prev); > > > > > if (!offset) > > > > > return; > > > > > pdev->config[prev] = pdev->config[offset + > > > > > PCI_CAP_LIST_NEXT]; > > > > > -- > > > > > 1.9.1 > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-28 6:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-25 9:59 [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability SeokYeon Hwang 2014-06-25 12:03 ` Michael S. Tsirkin 2014-06-25 12:28 ` Marcel Apfelbaum 2014-06-26 3:48 ` SeokYeon Hwang 2014-06-26 7:07 ` Michael S. Tsirkin 2014-06-28 6:39 ` SeokYeon Hwang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).