vp_dev->msix_vectors should be initialized before allocating
msix_affinity_masks, otherwise vp_free_vectors will not free these
objects.
unreferenced object 0xffff88010f969d88 (size 512):
comm "systemd-udevd", pid 158, jiffies 4294673645 (age 80.545s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff816e455e>] kmemleak_alloc+0x5e/0xc0
[<ffffffff811aa7f1>] kmem_cache_alloc_node_trace+0x141/0x2c0
[<ffffffff8133ba23>] alloc_cpumask_var_node+0x23/0x80
[<ffffffff8133ba8e>] alloc_cpumask_var+0xe/0x10
[<ffffffff813fdb3d>] vp_try_to_find_vqs+0x25d/0x810
[<ffffffff813fe171>] vp_find_vqs+0x81/0xb0
[<ffffffffa00d2a05>] init_vqs+0x85/0x120 [virtio_balloon]
[<ffffffffa00d2c29>] virtballoon_probe+0xf9/0x1a0 [virtio_balloon]
[<ffffffff813fb61e>] virtio_dev_probe+0xde/0x140
[<ffffffff814452b8>] driver_probe_device+0x98/0x3a0
[<ffffffff8144566b>] __driver_attach+0xab/0xb0
[<ffffffff814432f4>] bus_for_each_dev+0x94/0xb0
[<ffffffff81444f4e>] driver_attach+0x1e/0x20
[<ffffffff81444910>] bus_add_driver+0x200/0x280
[<ffffffff81445c14>] driver_register+0x74/0x160
[<ffffffff813fb7d0>] register_virtio_driver+0x20/0x40
Cc: Rusty Russell <[email protected]>
Cc: "Michael S. Tsirkin" <[email protected]>
Signed-off-by: Andrew Vagin <[email protected]>
---
drivers/virtio/virtio_pci.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index a7ce730..3c0a6ef 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -309,6 +309,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
unsigned i, v;
int err = -ENOMEM;
+ vp_dev->msix_vectors = nvectors;
+
vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
GFP_KERNEL);
if (!vp_dev->msix_entries)
@@ -336,7 +338,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
err = -ENOSPC;
if (err)
goto error;
- vp_dev->msix_vectors = nvectors;
vp_dev->msix_enabled = 1;
/* Set the vector used for configuration */
--
1.7.1
On Wed, Jun 19, 2013 at 07:59:48PM +0400, Andrew Vagin wrote:
> vp_dev->msix_vectors should be initialized before allocating
> msix_affinity_masks, otherwise vp_free_vectors will not free these
> objects.
>
> unreferenced object 0xffff88010f969d88 (size 512):
> comm "systemd-udevd", pid 158, jiffies 4294673645 (age 80.545s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff816e455e>] kmemleak_alloc+0x5e/0xc0
> [<ffffffff811aa7f1>] kmem_cache_alloc_node_trace+0x141/0x2c0
> [<ffffffff8133ba23>] alloc_cpumask_var_node+0x23/0x80
> [<ffffffff8133ba8e>] alloc_cpumask_var+0xe/0x10
> [<ffffffff813fdb3d>] vp_try_to_find_vqs+0x25d/0x810
> [<ffffffff813fe171>] vp_find_vqs+0x81/0xb0
> [<ffffffffa00d2a05>] init_vqs+0x85/0x120 [virtio_balloon]
> [<ffffffffa00d2c29>] virtballoon_probe+0xf9/0x1a0 [virtio_balloon]
> [<ffffffff813fb61e>] virtio_dev_probe+0xde/0x140
> [<ffffffff814452b8>] driver_probe_device+0x98/0x3a0
> [<ffffffff8144566b>] __driver_attach+0xab/0xb0
> [<ffffffff814432f4>] bus_for_each_dev+0x94/0xb0
> [<ffffffff81444f4e>] driver_attach+0x1e/0x20
> [<ffffffff81444910>] bus_add_driver+0x200/0x280
> [<ffffffff81445c14>] driver_register+0x74/0x160
> [<ffffffff813fb7d0>] register_virtio_driver+0x20/0x40
>
> Cc: Rusty Russell <[email protected]>
> Cc: "Michael S. Tsirkin" <[email protected]>
> Signed-off-by: Andrew Vagin <[email protected]>
> ---
> drivers/virtio/virtio_pci.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index a7ce730..3c0a6ef 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -309,6 +309,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> unsigned i, v;
> int err = -ENOMEM;
>
> + vp_dev->msix_vectors = nvectors;
> +
> vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
> GFP_KERNEL);
> if (!vp_dev->msix_entries)
> @@ -336,7 +338,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> err = -ENOSPC;
> if (err)
> goto error;
> - vp_dev->msix_vectors = nvectors;
> vp_dev->msix_enabled = 1;
>
> /* Set the vector used for configuration */
This introduces a bug.
The assumption was that vp_free_vectors is only set if
msix is successfully enabled, so it's not cleared
by vp_free_vectors.
So there are places like vp_synchronize_vectors
that assume that msix_vectors is only set if msix works
fine.
If you change the assumption, and assign msix_vectors even
if msix can later fail, need too clear it unconditionally.
Like this (untested)
Signed-off-by: Michael S. Tsirkin <[email protected]>
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 03564fe..4236d92 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -289,9 +289,9 @@ static void vp_free_vectors(struct virtio_device *vdev)
pci_disable_msix(vp_dev->pci_dev);
vp_dev->msix_enabled = 0;
- vp_dev->msix_vectors = 0;
}
+ vp_dev->msix_vectors = 0;
vp_dev->msix_used_vectors = 0;
kfree(vp_dev->msix_names);
vp_dev->msix_names = NULL;
> --
> 1.7.1
From: Andrew Vagin <[email protected]>
vp_dev->msix_vectors should be initialized before allocating
msix_affinity_masks, otherwise vp_free_vectors will not free these
objects.
unreferenced object 0xffff88010f969d88 (size 512):
comm "systemd-udevd", pid 158, jiffies 4294673645 (age 80.545s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff816e455e>] kmemleak_alloc+0x5e/0xc0
[<ffffffff811aa7f1>] kmem_cache_alloc_node_trace+0x141/0x2c0
[<ffffffff8133ba23>] alloc_cpumask_var_node+0x23/0x80
[<ffffffff8133ba8e>] alloc_cpumask_var+0xe/0x10
[<ffffffff813fdb3d>] vp_try_to_find_vqs+0x25d/0x810
[<ffffffff813fe171>] vp_find_vqs+0x81/0xb0
[<ffffffffa00d2a05>] init_vqs+0x85/0x120 [virtio_balloon]
[<ffffffffa00d2c29>] virtballoon_probe+0xf9/0x1a0 [virtio_balloon]
[<ffffffff813fb61e>] virtio_dev_probe+0xde/0x140
[<ffffffff814452b8>] driver_probe_device+0x98/0x3a0
[<ffffffff8144566b>] __driver_attach+0xab/0xb0
[<ffffffff814432f4>] bus_for_each_dev+0x94/0xb0
[<ffffffff81444f4e>] driver_attach+0x1e/0x20
[<ffffffff81444910>] bus_add_driver+0x200/0x280
[<ffffffff81445c14>] driver_register+0x74/0x160
[<ffffffff813fb7d0>] register_virtio_driver+0x20/0x40
v2: change msix_vectors uncoditionaly in vp_free_vectors
Cc: Rusty Russell <[email protected]>
Cc: "Michael S. Tsirkin" <[email protected]>
Cc: Jason Wang <[email protected]>
Signed-off-by: Andrew Vagin <[email protected]>
Signed-off-by: Andrey Vagin <[email protected]>
---
drivers/virtio/virtio_pci.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index a7ce730..1aba255 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -289,9 +289,9 @@ static void vp_free_vectors(struct virtio_device *vdev)
pci_disable_msix(vp_dev->pci_dev);
vp_dev->msix_enabled = 0;
- vp_dev->msix_vectors = 0;
}
+ vp_dev->msix_vectors = 0;
vp_dev->msix_used_vectors = 0;
kfree(vp_dev->msix_names);
vp_dev->msix_names = NULL;
@@ -309,6 +309,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
unsigned i, v;
int err = -ENOMEM;
+ vp_dev->msix_vectors = nvectors;
+
vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
GFP_KERNEL);
if (!vp_dev->msix_entries)
@@ -336,7 +338,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
err = -ENOSPC;
if (err)
goto error;
- vp_dev->msix_vectors = nvectors;
vp_dev->msix_enabled = 1;
/* Set the vector used for configuration */
--
1.8.1.4
On Thu, Jun 20, 2013 at 09:36:50AM +0400, Andrey Vagin wrote:
> From: Andrew Vagin <[email protected]>
>
> vp_dev->msix_vectors should be initialized before allocating
> msix_affinity_masks, otherwise vp_free_vectors will not free these
> objects.
>
> unreferenced object 0xffff88010f969d88 (size 512):
> comm "systemd-udevd", pid 158, jiffies 4294673645 (age 80.545s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff816e455e>] kmemleak_alloc+0x5e/0xc0
> [<ffffffff811aa7f1>] kmem_cache_alloc_node_trace+0x141/0x2c0
> [<ffffffff8133ba23>] alloc_cpumask_var_node+0x23/0x80
> [<ffffffff8133ba8e>] alloc_cpumask_var+0xe/0x10
> [<ffffffff813fdb3d>] vp_try_to_find_vqs+0x25d/0x810
> [<ffffffff813fe171>] vp_find_vqs+0x81/0xb0
> [<ffffffffa00d2a05>] init_vqs+0x85/0x120 [virtio_balloon]
> [<ffffffffa00d2c29>] virtballoon_probe+0xf9/0x1a0 [virtio_balloon]
> [<ffffffff813fb61e>] virtio_dev_probe+0xde/0x140
> [<ffffffff814452b8>] driver_probe_device+0x98/0x3a0
> [<ffffffff8144566b>] __driver_attach+0xab/0xb0
> [<ffffffff814432f4>] bus_for_each_dev+0x94/0xb0
> [<ffffffff81444f4e>] driver_attach+0x1e/0x20
> [<ffffffff81444910>] bus_add_driver+0x200/0x280
> [<ffffffff81445c14>] driver_register+0x74/0x160
> [<ffffffff813fb7d0>] register_virtio_driver+0x20/0x40
>
> v2: change msix_vectors uncoditionaly in vp_free_vectors
>
> Cc: Rusty Russell <[email protected]>
> Cc: "Michael S. Tsirkin" <[email protected]>
> Cc: Jason Wang <[email protected]>
> Signed-off-by: Andrew Vagin <[email protected]>
> Signed-off-by: Andrey Vagin <[email protected]>
Acked-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/virtio/virtio_pci.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index a7ce730..1aba255 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -289,9 +289,9 @@ static void vp_free_vectors(struct virtio_device *vdev)
>
> pci_disable_msix(vp_dev->pci_dev);
> vp_dev->msix_enabled = 0;
> - vp_dev->msix_vectors = 0;
> }
>
> + vp_dev->msix_vectors = 0;
> vp_dev->msix_used_vectors = 0;
> kfree(vp_dev->msix_names);
> vp_dev->msix_names = NULL;
> @@ -309,6 +309,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> unsigned i, v;
> int err = -ENOMEM;
>
> + vp_dev->msix_vectors = nvectors;
> +
> vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
> GFP_KERNEL);
> if (!vp_dev->msix_entries)
> @@ -336,7 +338,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> err = -ENOSPC;
> if (err)
> goto error;
> - vp_dev->msix_vectors = nvectors;
> vp_dev->msix_enabled = 1;
>
> /* Set the vector used for configuration */
> --
> 1.8.1.4
On 06/20/2013 01:36 PM, Andrey Vagin wrote:
> From: Andrew Vagin <[email protected]>
>
> vp_dev->msix_vectors should be initialized before allocating
> msix_affinity_masks, otherwise vp_free_vectors will not free these
> objects.
>
> unreferenced object 0xffff88010f969d88 (size 512):
> comm "systemd-udevd", pid 158, jiffies 4294673645 (age 80.545s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff816e455e>] kmemleak_alloc+0x5e/0xc0
> [<ffffffff811aa7f1>] kmem_cache_alloc_node_trace+0x141/0x2c0
> [<ffffffff8133ba23>] alloc_cpumask_var_node+0x23/0x80
> [<ffffffff8133ba8e>] alloc_cpumask_var+0xe/0x10
> [<ffffffff813fdb3d>] vp_try_to_find_vqs+0x25d/0x810
> [<ffffffff813fe171>] vp_find_vqs+0x81/0xb0
> [<ffffffffa00d2a05>] init_vqs+0x85/0x120 [virtio_balloon]
> [<ffffffffa00d2c29>] virtballoon_probe+0xf9/0x1a0 [virtio_balloon]
> [<ffffffff813fb61e>] virtio_dev_probe+0xde/0x140
> [<ffffffff814452b8>] driver_probe_device+0x98/0x3a0
> [<ffffffff8144566b>] __driver_attach+0xab/0xb0
> [<ffffffff814432f4>] bus_for_each_dev+0x94/0xb0
> [<ffffffff81444f4e>] driver_attach+0x1e/0x20
> [<ffffffff81444910>] bus_add_driver+0x200/0x280
> [<ffffffff81445c14>] driver_register+0x74/0x160
> [<ffffffff813fb7d0>] register_virtio_driver+0x20/0x40
>
> v2: change msix_vectors uncoditionaly in vp_free_vectors
>
> Cc: Rusty Russell <[email protected]>
> Cc: "Michael S. Tsirkin" <[email protected]>
> Cc: Jason Wang <[email protected]>
> Signed-off-by: Andrew Vagin <[email protected]>
> Signed-off-by: Andrey Vagin <[email protected]>
> ---
> drivers/virtio/virtio_pci.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index a7ce730..1aba255 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -289,9 +289,9 @@ static void vp_free_vectors(struct virtio_device *vdev)
>
> pci_disable_msix(vp_dev->pci_dev);
> vp_dev->msix_enabled = 0;
> - vp_dev->msix_vectors = 0;
> }
>
> + vp_dev->msix_vectors = 0;
> vp_dev->msix_used_vectors = 0;
> kfree(vp_dev->msix_names);
> vp_dev->msix_names = NULL;
> @@ -309,6 +309,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> unsigned i, v;
> int err = -ENOMEM;
>
> + vp_dev->msix_vectors = nvectors;
> +
> vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
> GFP_KERNEL);
> if (!vp_dev->msix_entries)
> @@ -336,7 +338,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> err = -ENOSPC;
> if (err)
> goto error;
> - vp_dev->msix_vectors = nvectors;
> vp_dev->msix_enabled = 1;
>
> /* Set the vector used for configuration */
Acked-by: Jason Wang <[email protected]>
Jason Wang <[email protected]> writes:
> On 06/20/2013 01:36 PM, Andrey Vagin wrote:
>> From: Andrew Vagin <[email protected]>
>>
>> vp_dev->msix_vectors should be initialized before allocating
>> msix_affinity_masks, otherwise vp_free_vectors will not free these
>> objects.
>>
>> unreferenced object 0xffff88010f969d88 (size 512):
>> comm "systemd-udevd", pid 158, jiffies 4294673645 (age 80.545s)
>> hex dump (first 32 bytes):
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> backtrace:
>> [<ffffffff816e455e>] kmemleak_alloc+0x5e/0xc0
>> [<ffffffff811aa7f1>] kmem_cache_alloc_node_trace+0x141/0x2c0
>> [<ffffffff8133ba23>] alloc_cpumask_var_node+0x23/0x80
>> [<ffffffff8133ba8e>] alloc_cpumask_var+0xe/0x10
>> [<ffffffff813fdb3d>] vp_try_to_find_vqs+0x25d/0x810
>> [<ffffffff813fe171>] vp_find_vqs+0x81/0xb0
>> [<ffffffffa00d2a05>] init_vqs+0x85/0x120 [virtio_balloon]
>> [<ffffffffa00d2c29>] virtballoon_probe+0xf9/0x1a0 [virtio_balloon]
>> [<ffffffff813fb61e>] virtio_dev_probe+0xde/0x140
>> [<ffffffff814452b8>] driver_probe_device+0x98/0x3a0
>> [<ffffffff8144566b>] __driver_attach+0xab/0xb0
>> [<ffffffff814432f4>] bus_for_each_dev+0x94/0xb0
>> [<ffffffff81444f4e>] driver_attach+0x1e/0x20
>> [<ffffffff81444910>] bus_add_driver+0x200/0x280
>> [<ffffffff81445c14>] driver_register+0x74/0x160
>> [<ffffffff813fb7d0>] register_virtio_driver+0x20/0x40
>>
>> v2: change msix_vectors uncoditionaly in vp_free_vectors
>>
>> Cc: Rusty Russell <[email protected]>
>> Cc: "Michael S. Tsirkin" <[email protected]>
>> Cc: Jason Wang <[email protected]>
>> Signed-off-by: Andrew Vagin <[email protected]>
>> Signed-off-by: Andrey Vagin <[email protected]>
>> ---
>> drivers/virtio/virtio_pci.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
Thanks, applied.
Cheers,
Rusty.
>>
>> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
>> index a7ce730..1aba255 100644
>> --- a/drivers/virtio/virtio_pci.c
>> +++ b/drivers/virtio/virtio_pci.c
>> @@ -289,9 +289,9 @@ static void vp_free_vectors(struct virtio_device *vdev)
>>
>> pci_disable_msix(vp_dev->pci_dev);
>> vp_dev->msix_enabled = 0;
>> - vp_dev->msix_vectors = 0;
>> }
>>
>> + vp_dev->msix_vectors = 0;
>> vp_dev->msix_used_vectors = 0;
>> kfree(vp_dev->msix_names);
>> vp_dev->msix_names = NULL;
>> @@ -309,6 +309,8 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>> unsigned i, v;
>> int err = -ENOMEM;
>>
>> + vp_dev->msix_vectors = nvectors;
>> +
>> vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
>> GFP_KERNEL);
>> if (!vp_dev->msix_entries)
>> @@ -336,7 +338,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
>> err = -ENOSPC;
>> if (err)
>> goto error;
>> - vp_dev->msix_vectors = nvectors;
>> vp_dev->msix_enabled = 1;
>>
>> /* Set the vector used for configuration */
> Acked-by: Jason Wang <[email protected]>