virtio: fix double free_irq on device removal

PATCH Applied Michael S. Tsirkin 2 participants 8 messages Jul 23, 2009 – Jul 24, 2009 1 message not in the archive

Subsystems (2)

KVM virtio core

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael S. Tsirkin

author maintainer

2009-07-23 11:57 UTC #1 lore raw

PATCH v2 1/2 virtio: fix double free_irq on device removal

msix_user_vectors counted both per-vq and shared/config vectors. This causes BUG_ON when device is removed, as free_vectors tries to free per-vq vectors.

Count per-vq vectors separately so they are only freed by del_vq.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_pci.c |   38 +++++++++++++++++++++++---------------
 1 files changed, 23 insertions(+), 15 deletions(-)

drivers/virtio/virtio_pci.c +23 ~~-15~~

index 193c8f0..a40e4f7 100644
@@ -52,7 +52,7 @@ struct virtio_pci_device
 	char (*msix_names)[256];
 	/* Number of available vectors */
 	unsigned msix_vectors;
-	/* Vectors allocated */
+	/* Vectors allocated, excluding per-vq vectors if any */
 	unsigned msix_used_vectors;
 };

@@ -364,13 +364,15 @@ error_entries:
 
 static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 				    void (*callback)(struct virtqueue *vq),
-				    const char *name)
+				    const char *name,
+				    u16 vector,
+				    u16 per_vq_vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
 	unsigned long flags, size;
-	u16 num, vector;
+	u16 num;
 	int err;
 
 	/* Select the queue we're interested in */

@@ -389,7 +391,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 
 	info->queue_index = index;
 	info->num = num;
-	info->vector = VIRTIO_MSI_NO_VECTOR;
+	info->vector = per_vq_vector;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);

@@ -414,8 +416,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	info->vq = vq;
 
 	/* allocate per-vq vector if available and necessary */
-	if (callback && vp_dev->msix_used_vectors < vp_dev->msix_vectors) {
-		vector = vp_dev->msix_used_vectors;
+	if (info->vector != VIRTIO_MSI_NO_VECTOR) {
 		snprintf(vp_dev->msix_names[vector], sizeof *vp_dev->msix_names,
 			 "%s-%s", dev_name(&vp_dev->vdev.dev), name);
 		err = request_irq(vp_dev->msix_entries[vector].vector,

@@ -423,10 +424,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 				  vp_dev->msix_names[vector], vq);
 		if (err)
 			goto out_request_irq;
-		info->vector = vector;
-		++vp_dev->msix_used_vectors;
-	} else
-		vector = VP_MSIX_VQ_VECTOR;
+	}
 
 	 if (callback && vp_dev->msix_enabled) {
 		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);

@@ -444,10 +442,8 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_assign:
-	if (info->vector != VIRTIO_MSI_NO_VECTOR) {
+	if (info->vector != VIRTIO_MSI_NO_VECTOR)
 		free_irq(vp_dev->msix_entries[info->vector].vector, vq);
-		--vp_dev->msix_used_vectors;
-	}
 out_request_irq:
 	vring_del_virtqueue(vq);
 out_activate_queue:

@@ -503,8 +499,10 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		       vq_callback_t *callbacks[],
 		       const char *names[])
 {
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	u16 vector, per_vq_vector;
 	int vectors = 0;
-	int i, err;
+	int i, err, allocated_vectors;
 
 	/* How many vectors would we like? */
 	for (i = 0; i < nvqs; ++i)

@@ -515,8 +513,18 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 	if (err)
 		goto error_request;
 
+	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i]);
+		if (!callbacks[i])
+			vector = per_vq_vector = VIRTIO_MSI_NO_VECTOR;
+		else if (vp_dev->msix_used_vectors < vp_dev->msix_vectors)
+			per_vq_vector = vector = allocated_vectors++;
+		else {
+			vector = VP_MSIX_VQ_VECTOR;
+			per_vq_vector = VIRTIO_MSI_NO_VECTOR;
+		}
+		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i],
+				    vector, per_vq_vector);
 		if (IS_ERR(vqs[i]))
 			goto error_find;
 	}

Signature · 1 line

1.6.2.5

Michael S. Tsirkin

author maintainer

2009-07-23 11:58 UTC #2 lore raw

PATCH v2 1/2 virtio: fix double free_irq on device removal

In reply to a message not in the archive

msix_user_vectors counted both per-vq and shared/config vectors. This causes BUG_ON when device is removed, as free_vectors tries to free per-vq vectors.

Count per-vq vectors separately so they are only freed by del_vq.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_pci.c |   38 +++++++++++++++++++++++---------------
 1 files changed, 23 insertions(+), 15 deletions(-)

drivers/virtio/virtio_pci.c +23 ~~-15~~

index 193c8f0..a40e4f7 100644
@@ -52,7 +52,7 @@ struct virtio_pci_device
 	char (*msix_names)[256];
 	/* Number of available vectors */
 	unsigned msix_vectors;
-	/* Vectors allocated */
+	/* Vectors allocated, excluding per-vq vectors if any */
 	unsigned msix_used_vectors;
 };

@@ -364,13 +364,15 @@ error_entries:
 
 static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 				    void (*callback)(struct virtqueue *vq),
-				    const char *name)
+				    const char *name,
+				    u16 vector,
+				    u16 per_vq_vector)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
 	unsigned long flags, size;
-	u16 num, vector;
+	u16 num;
 	int err;
 
 	/* Select the queue we're interested in */

@@ -389,7 +391,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 
 	info->queue_index = index;
 	info->num = num;
-	info->vector = VIRTIO_MSI_NO_VECTOR;
+	info->vector = per_vq_vector;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
 	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);

@@ -414,8 +416,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	info->vq = vq;
 
 	/* allocate per-vq vector if available and necessary */
-	if (callback && vp_dev->msix_used_vectors < vp_dev->msix_vectors) {
-		vector = vp_dev->msix_used_vectors;
+	if (info->vector != VIRTIO_MSI_NO_VECTOR) {
 		snprintf(vp_dev->msix_names[vector], sizeof *vp_dev->msix_names,
 			 "%s-%s", dev_name(&vp_dev->vdev.dev), name);
 		err = request_irq(vp_dev->msix_entries[vector].vector,

@@ -423,10 +424,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 				  vp_dev->msix_names[vector], vq);
 		if (err)
 			goto out_request_irq;
-		info->vector = vector;
-		++vp_dev->msix_used_vectors;
-	} else
-		vector = VP_MSIX_VQ_VECTOR;
+	}
 
 	 if (callback && vp_dev->msix_enabled) {
 		iowrite16(vector, vp_dev->ioaddr + VIRTIO_MSI_QUEUE_VECTOR);

@@ -444,10 +442,8 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_assign:
-	if (info->vector != VIRTIO_MSI_NO_VECTOR) {
+	if (info->vector != VIRTIO_MSI_NO_VECTOR)
 		free_irq(vp_dev->msix_entries[info->vector].vector, vq);
-		--vp_dev->msix_used_vectors;
-	}
 out_request_irq:
 	vring_del_virtqueue(vq);
 out_activate_queue:

@@ -503,8 +499,10 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		       vq_callback_t *callbacks[],
 		       const char *names[])
 {
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	u16 vector, per_vq_vector;
 	int vectors = 0;
-	int i, err;
+	int i, err, allocated_vectors;
 
 	/* How many vectors would we like? */
 	for (i = 0; i < nvqs; ++i)

@@ -515,8 +513,18 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 	if (err)
 		goto error_request;
 
+	allocated_vectors = vp_dev->msix_used_vectors;
 	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i]);
+		if (!callbacks[i])
+			vector = per_vq_vector = VIRTIO_MSI_NO_VECTOR;
+		else if (vp_dev->msix_used_vectors < vp_dev->msix_vectors)
+			per_vq_vector = vector = allocated_vectors++;
+		else {
+			vector = VP_MSIX_VQ_VECTOR;
+			per_vq_vector = VIRTIO_MSI_NO_VECTOR;
+		}
+		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i],
+				    vector, per_vq_vector);
 		if (IS_ERR(vqs[i]))
 			goto error_find;
 	}

Signature · 1 line

1.6.2.5

Rusty Russell

2009-07-24 12:24 UTC #3 lore raw

Quoted 3 lines · Michael S. Tsirkin

msix_user_vectors counted both per-vq and shared/config vectors. This causes BUG_ON when device is removed, as free_vectors tries to free per-vq vectors.

OK, I looked at this patch, then looked at this code (after it was applied).

I'm still very confused.

Looking at the call site for vp_find_vq:

	for (i = 0; i < nvqs; ++i) {
		if (!callbacks[i])
			vector = per_vq_vector = VIRTIO_MSI_NO_VECTOR;
		else if (vp_dev->msix_used_vectors < vp_dev->msix_vectors)
			per_vq_vector = vector = allocated_vectors++;
		else {
			vector = VP_MSIX_VQ_VECTOR;
			per_vq_vector = VIRTIO_MSI_NO_VECTOR;
		}

Now, I can't find where msix_used_vectors is set, only once where it's incremented. It seems completely redundant and confusing now? And this "< vp_dev->msix_vectors" test is wrong?

AFAICT there are three cases:
1) We don't have MSI, so we use a normal interrupt for all vqs (old style). This request_irq is done in vp_request_vectors.
2) We get some, but not enough for one per queue. We then use 2: one for changes, and one for all vqs. Requested in vp_request_vectors.
3) We get enough. Use one for changes, one per vq. Each vq requests in vp_find_vq.

I suggest you be explicit, and don't do any request in vp_find_vq(). Do the per-vq request (case 3) in the find_vqs() loop, so vp_find_vq doesn't need to know anything except to do the iowrite16 if vector != VIRTIO_MSI_NO_VECTOR.

Maybe an explicit "bool per_vq_vectors" would make it clearer, too.

Note: this is partially my fault for not reviewing this code when it went in. I know Anthony is disclaiming virtio_pci :)

Thanks,
Rusty.

Rusty Russell

2009-07-24 12:24 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 3 lines · Michael S. Tsirkin

msix_user_vectors counted both per-vq and shared/config vectors. This causes BUG_ON when device is removed, as free_vectors tries to free per-vq vectors.

OK, I looked at this patch, then looked at this code (after it was applied).

I'm still very confused.

Looking at the call site for vp_find_vq:

	for (i = 0; i < nvqs; ++i) {
		if (!callbacks[i])
			vector = per_vq_vector = VIRTIO_MSI_NO_VECTOR;
		else if (vp_dev->msix_used_vectors < vp_dev->msix_vectors)
			per_vq_vector = vector = allocated_vectors++;
		else {
			vector = VP_MSIX_VQ_VECTOR;
			per_vq_vector = VIRTIO_MSI_NO_VECTOR;
		}

Now, I can't find where msix_used_vectors is set, only once where it's incremented. It seems completely redundant and confusing now? And this "< vp_dev->msix_vectors" test is wrong?

Maybe an explicit "bool per_vq_vectors" would make it clearer, too.

Note: this is partially my fault for not reviewing this code when it went in. I know Anthony is disclaiming virtio_pci :)

Thanks,
Rusty.

Michael S. Tsirkin

author maintainer

2009-07-23 11:57 UTC #5 lore raw

PATCH v2 2/2 virtio: fix memory leak on device removal

In reply to a message not in the archive

Make vp_free_vectors do the reverse of vq_request_vectors.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_pci.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

drivers/virtio/virtio_pci.c +15 ~~-13~~

index a40e4f7..3aff895 100644
@@ -258,7 +258,6 @@ static void vp_free_vectors(struct virtio_device *vdev)
 
 	for (i = 0; i < vp_dev->msix_used_vectors; ++i)
 		free_irq(vp_dev->msix_entries[i].vector, vp_dev);
-	vp_dev->msix_used_vectors = 0;
 
 	if (vp_dev->msix_enabled) {
 		/* Disable the vector used for configuration */

@@ -267,9 +266,16 @@ static void vp_free_vectors(struct virtio_device *vdev)
 		/* Flush the write out to device */
 		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
 
-		vp_dev->msix_enabled = 0;
 		pci_disable_msix(vp_dev->pci_dev);
+		vp_dev->msix_enabled = 0;
+		vp_dev->msix_vectors = 0;
 	}
+
+	vp_dev->msix_used_vectors = 0;
+	kfree(vp_dev->msix_names);
+	vp_dev->msix_names = NULL;
+	kfree(vp_dev->msix_entries);
+	vp_dev->msix_entries = NULL;
 }
 
 static int vp_enable_msix(struct pci_dev *dev, struct msix_entry *entries,

@@ -297,11 +303,11 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
 				       GFP_KERNEL);
 	if (!vp_dev->msix_entries)
-		goto error_entries;
+		goto error;
 	vp_dev->msix_names = kmalloc(nvectors * sizeof *vp_dev->msix_names,
 				     GFP_KERNEL);
 	if (!vp_dev->msix_names)
-		goto error_names;
+		goto error;
 
 	for (i = 0; i < nvectors; ++i)
 		vp_dev->msix_entries[i].entry = i;

@@ -314,7 +320,7 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, name, vp_dev);
 		if (err)
-			goto error_irq;
+			goto error;
 		vp_dev->intx_enabled = 1;
 	} else {
 		vp_dev->msix_vectors = err;

@@ -328,7 +334,7 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 				  vp_config_changed, 0, vp_dev->msix_names[v],
 				  vp_dev);
 		if (err)
-			goto error_irq;
+			goto error;
 		++vp_dev->msix_used_vectors;
 
 		iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);

@@ -336,7 +342,7 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 		v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
 		if (v == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
-			goto error_irq;
+			goto error;
 		}
 	}

@@ -349,16 +355,12 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 				  vp_vring_interrupt, 0, vp_dev->msix_names[v],
 				  vp_dev);
 		if (err)
-			goto error_irq;
+			goto error;
 		++vp_dev->msix_used_vectors;
 	}
 	return 0;
-error_irq:
+error:
 	vp_free_vectors(vdev);
-	kfree(vp_dev->msix_names);
-error_names:
-	kfree(vp_dev->msix_entries);
-error_entries:
 	return err;
 }

Signature · 1 line

1.6.2.5

Michael S. Tsirkin

author maintainer

2009-07-23 11:58 UTC #6 lore raw

PATCH v2 2/2 virtio: fix memory leak on device removal

In reply to a message not in the archive

Make vp_free_vectors do the reverse of vq_request_vectors.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/virtio/virtio_pci.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

drivers/virtio/virtio_pci.c +15 ~~-13~~

index a40e4f7..3aff895 100644
@@ -258,7 +258,6 @@ static void vp_free_vectors(struct virtio_device *vdev)
 
 	for (i = 0; i < vp_dev->msix_used_vectors; ++i)
 		free_irq(vp_dev->msix_entries[i].vector, vp_dev);
-	vp_dev->msix_used_vectors = 0;
 
 	if (vp_dev->msix_enabled) {
 		/* Disable the vector used for configuration */

@@ -267,9 +266,16 @@ static void vp_free_vectors(struct virtio_device *vdev)
 		/* Flush the write out to device */
 		ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
 
-		vp_dev->msix_enabled = 0;
 		pci_disable_msix(vp_dev->pci_dev);
+		vp_dev->msix_enabled = 0;
+		vp_dev->msix_vectors = 0;
 	}
+
+	vp_dev->msix_used_vectors = 0;
+	kfree(vp_dev->msix_names);
+	vp_dev->msix_names = NULL;
+	kfree(vp_dev->msix_entries);
+	vp_dev->msix_entries = NULL;
 }
 
 static int vp_enable_msix(struct pci_dev *dev, struct msix_entry *entries,

@@ -297,11 +303,11 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 	vp_dev->msix_entries = kmalloc(nvectors * sizeof *vp_dev->msix_entries,
 				       GFP_KERNEL);
 	if (!vp_dev->msix_entries)
-		goto error_entries;
+		goto error;
 	vp_dev->msix_names = kmalloc(nvectors * sizeof *vp_dev->msix_names,
 				     GFP_KERNEL);
 	if (!vp_dev->msix_names)
-		goto error_names;
+		goto error;
 
 	for (i = 0; i < nvectors; ++i)
 		vp_dev->msix_entries[i].entry = i;

@@ -314,7 +320,7 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 		err = request_irq(vp_dev->pci_dev->irq, vp_interrupt,
 				  IRQF_SHARED, name, vp_dev);
 		if (err)
-			goto error_irq;
+			goto error;
 		vp_dev->intx_enabled = 1;
 	} else {
 		vp_dev->msix_vectors = err;

@@ -328,7 +334,7 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 				  vp_config_changed, 0, vp_dev->msix_names[v],
 				  vp_dev);
 		if (err)
-			goto error_irq;
+			goto error;
 		++vp_dev->msix_used_vectors;
 
 		iowrite16(v, vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);

@@ -336,7 +342,7 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 		v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
 		if (v == VIRTIO_MSI_NO_VECTOR) {
 			err = -EBUSY;
-			goto error_irq;
+			goto error;
 		}
 	}

@@ -349,16 +355,12 @@ static int vp_request_vectors(struct virtio_device *vdev, unsigned max_vqs)
 				  vp_vring_interrupt, 0, vp_dev->msix_names[v],
 				  vp_dev);
 		if (err)
-			goto error_irq;
+			goto error;
 		++vp_dev->msix_used_vectors;
 	}
 	return 0;
-error_irq:
+error:
 	vp_free_vectors(vdev);
-	kfree(vp_dev->msix_names);
-error_names:
-	kfree(vp_dev->msix_entries);
-error_entries:
 	return err;
 }

Signature · 1 line

1.6.2.5

Rusty Russell

Applied

2009-07-24 12:25 UTC #7 lore raw

Re: [PATCHv2 2/2] virtio: fix memory leak on device removal

Quoted 3 lines · Michael S. Tsirkin

Make vp_free_vectors do the reverse of vq_request_vectors.
Signed-off-by Michael S. Tsirkin <[email protected]>

Thanks, applied!

Rusty.

Rusty Russell

Applied

2009-07-24 12:25 UTC #8 lore raw

Re: [PATCHv2 2/2] virtio: fix memory leak on device removal

In reply to Michael S. Tsirkin

Quoted 3 lines · Michael S. Tsirkin

Make vp_free_vectors do the reverse of vq_request_vectors.
Signed-off-by Michael S. Tsirkin <[email protected]>

Thanks, applied!

Rusty.