* [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs
@ 2015-01-02 19:47 Sasha Levin
2015-01-02 19:47 ` [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released Sasha Levin
2015-01-04 11:12 ` [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs Michael S. Tsirkin
0 siblings, 2 replies; 6+ messages in thread
From: Sasha Levin @ 2015-01-02 19:47 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, open list:VIRTIO CORE, NET..., Michael S. Tsirkin
Device VQs were getting freed twice: once in every devices removal functions,
and then again in virtio_pci_legacy_remove().
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
drivers/virtio/virtio_pci_legacy.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 6c76f0f..913ca23 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -298,7 +298,6 @@ void virtio_pci_legacy_remove(struct pci_dev *pci_dev)
unregister_virtio_device(&vp_dev->vdev);
- vp_del_vqs(&vp_dev->vdev);
pci_iounmap(pci_dev, vp_dev->ioaddr);
pci_release_regions(pci_dev);
pci_disable_device(pci_dev);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released
2015-01-02 19:47 [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs Sasha Levin
@ 2015-01-02 19:47 ` Sasha Levin
2015-01-04 11:26 ` Michael S. Tsirkin
2015-01-04 11:12 ` [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs Michael S. Tsirkin
1 sibling, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2015-01-02 19:47 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, open list:VIRTIO CORE, NET..., Michael S. Tsirkin
When releasing a virtio device, We can't free a struct virtio_device until the
underlying struct device has been released, which might not happen immediately
on device_unregister() even if that was the device's last reference.
Instead, free the memory only once we know the device is gone in the release
callback.
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
drivers/virtio/virtio_pci_common.c | 9 ++++-----
drivers/virtio/virtio_pci_legacy.c | 1 -
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 59d3685..caa483d 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -423,11 +423,10 @@ int vp_set_vq_affinity(struct virtqueue *vq, int cpu)
void virtio_pci_release_dev(struct device *_d)
{
- /*
- * No need for a release method as we allocate/free
- * all devices together with the pci devices.
- * Provide an empty one to avoid getting a warning from core.
- */
+ struct virtio_device *vdev = dev_to_virtio(_d);
+ struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+
+ kfree(vp_dev);
}
#ifdef CONFIG_PM_SLEEP
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 913ca23..15e6e6d 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -301,5 +301,4 @@ void virtio_pci_legacy_remove(struct pci_dev *pci_dev)
pci_iounmap(pci_dev, vp_dev->ioaddr);
pci_release_regions(pci_dev);
pci_disable_device(pci_dev);
- kfree(vp_dev);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs
2015-01-02 19:47 [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs Sasha Levin
2015-01-02 19:47 ` [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released Sasha Levin
@ 2015-01-04 11:12 ` Michael S. Tsirkin
1 sibling, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-01-04 11:12 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-kernel, open list:VIRTIO CORE, NET...
On Fri, Jan 02, 2015 at 02:47:39PM -0500, Sasha Levin wrote:
> Device VQs were getting freed twice: once in every devices removal functions,
> and then again in virtio_pci_legacy_remove().
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
I see. This used to be harmless because it used to be idempotent,
and safe to call any number of times.
I think your patch is right for 3.20:
Acked-by: Michael S. Tsirkin <mst@redhat.com>
but for 3.19 I'd prefer a more conservative approach of making
del_vqs idempotent again.
I'll post a patch like this shortly.
> ---
> drivers/virtio/virtio_pci_legacy.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 6c76f0f..913ca23 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -298,7 +298,6 @@ void virtio_pci_legacy_remove(struct pci_dev *pci_dev)
>
> unregister_virtio_device(&vp_dev->vdev);
>
> - vp_del_vqs(&vp_dev->vdev);
> pci_iounmap(pci_dev, vp_dev->ioaddr);
> pci_release_regions(pci_dev);
> pci_disable_device(pci_dev);
> --
> 1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released
2015-01-02 19:47 ` [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released Sasha Levin
@ 2015-01-04 11:26 ` Michael S. Tsirkin
2015-01-04 15:03 ` Sasha Levin
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-01-04 11:26 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-kernel, open list:VIRTIO CORE, NET...
On Fri, Jan 02, 2015 at 02:47:40PM -0500, Sasha Levin wrote:
> When releasing a virtio device, We can't free a struct virtio_device until the
> underlying struct device has been released, which might not happen immediately
> on device_unregister() even if that was the device's last reference.
>
> Instead, free the memory only once we know the device is gone in the release
> callback.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
Isn't this an old bug: do we need to copy stable on a fix?
What is the behaviour without this patch?
Is there a way to make this cause a crash?
> ---
> drivers/virtio/virtio_pci_common.c | 9 ++++-----
> drivers/virtio/virtio_pci_legacy.c | 1 -
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 59d3685..caa483d 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -423,11 +423,10 @@ int vp_set_vq_affinity(struct virtqueue *vq, int cpu)
>
> void virtio_pci_release_dev(struct device *_d)
> {
> - /*
> - * No need for a release method as we allocate/free
> - * all devices together with the pci devices.
> - * Provide an empty one to avoid getting a warning from core.
> - */
> + struct virtio_device *vdev = dev_to_virtio(_d);
> + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +
> + kfree(vp_dev);
> }
>
> #ifdef CONFIG_PM_SLEEP
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 913ca23..15e6e6d 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -301,5 +301,4 @@ void virtio_pci_legacy_remove(struct pci_dev *pci_dev)
> pci_iounmap(pci_dev, vp_dev->ioaddr);
> pci_release_regions(pci_dev);
> pci_disable_device(pci_dev);
> - kfree(vp_dev);
> }
It seems inelegant to free a structure allocated in another file:
I think we should move this function to virtio_pci_legacy.
Will send a patch in a minute.
> --
> 1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released
2015-01-04 11:26 ` Michael S. Tsirkin
@ 2015-01-04 15:03 ` Sasha Levin
2015-01-04 15:40 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2015-01-04 15:03 UTC (permalink / raw)
To: Michael S. Tsirkin, Sasha Levin
Cc: linux-kernel, open list:VIRTIO CORE, NET...
On 01/04/2015 06:26 AM, Michael S. Tsirkin wrote:
> On Fri, Jan 02, 2015 at 02:47:40PM -0500, Sasha Levin wrote:
>> > When releasing a virtio device, We can't free a struct virtio_device until the
>> > underlying struct device has been released, which might not happen immediately
>> > on device_unregister() even if that was the device's last reference.
>> >
>> > Instead, free the memory only once we know the device is gone in the release
>> > callback.
>> >
>> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>
> Isn't this an old bug: do we need to copy stable on a fix?
>
> What is the behaviour without this patch?
> Is there a way to make this cause a crash?
It will oops if CONFIG_DEBUG_KOBJECT_RELEASE is enabled. I guess
it needs to go to stable as well.
Thanks,
Sasha
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released
2015-01-04 15:03 ` Sasha Levin
@ 2015-01-04 15:40 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2015-01-04 15:40 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-kernel, open list:VIRTIO CORE, NET...
On Sun, Jan 04, 2015 at 10:03:27AM -0500, Sasha Levin wrote:
> On 01/04/2015 06:26 AM, Michael S. Tsirkin wrote:
> > On Fri, Jan 02, 2015 at 02:47:40PM -0500, Sasha Levin wrote:
> >> > When releasing a virtio device, We can't free a struct virtio_device until the
> >> > underlying struct device has been released, which might not happen immediately
> >> > on device_unregister() even if that was the device's last reference.
> >> >
> >> > Instead, free the memory only once we know the device is gone in the release
> >> > callback.
> >> >
> >> > Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> >
> > Isn't this an old bug: do we need to copy stable on a fix?
> >
> > What is the behaviour without this patch?
> > Is there a way to make this cause a crash?
>
> It will oops if CONFIG_DEBUG_KOBJECT_RELEASE is enabled. I guess
> it needs to go to stable as well.
>
>
> Thanks,
> Sasha
I see.
It's probably a good idea to document the crash in the commit log,
and also add a code comment.
I'll post the series with these minor tweaks, please confirm
that this fixes the issues for you.
--
MST
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-01-04 15:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-02 19:47 [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs Sasha Levin
2015-01-02 19:47 ` [PATCH 2/2] virtio: don't free memory until the underlying struct device has been released Sasha Levin
2015-01-04 11:26 ` Michael S. Tsirkin
2015-01-04 15:03 ` Sasha Levin
2015-01-04 15:40 ` Michael S. Tsirkin
2015-01-04 11:12 ` [PATCH 1/2] virtio_pci: double free and invalid memory access of device vqs Michael S. Tsirkin
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).