virtio_balloon: leak_balloon(): only tell host if we got pages deflated

PATCH In review Luiz Capitulino 2 participants 4 messages Jun 6, 2013 – Jun 17, 2013

Subsystems (4)

Memory management KVM virtio core virtio balloon

lore

Share & tools

lore

Luiz Capitulino

author

2013-06-06 01:18 UTC #0 lore raw

PATCH v2 1/1 virtio_balloon: leak_balloon(): only tell host if we got pages deflated

The balloon_page_dequeue() function can return NULL. If it does for the first page being freed, then leak_balloon() will create a scatter list with len=0. Which in turn seems to generate an invalid virtio request.

I didn't get this in practice, I found it by code review. On the other hand, such an invalid virtio request will cause errors in QEMU and fill_balloon() also performs the same check implemented by this commit.

Signed-off-by Luiz Capitulino <[email protected]>

Acked-by Rafael Aquini <[email protected]>

---

o v2

- Improve changelog

 drivers/virtio/virtio_balloon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/virtio/virtio_balloon.c +2 -1

index bd3ae32..71af7b5 100644
@@ -191,7 +191,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 	 * is true, we *have* to do it in this order
 	 */
-	tell_host(vb, vb->deflate_vq);
+	if (vb->num_pfns != 0)
+		tell_host(vb, vb->deflate_vq);
 	mutex_unlock(&vb->balloon_lock);
 	release_pages_by_pfn(vb->pfns, vb->num_pfns);
 }

Signature · 7 lines

1.8.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Rafael Aquini

2013-06-06 14:13 UTC #1 lore raw

Quoted 25 lines · Luiz Capitulino

The balloon_page_dequeue() function can return NULL. If it does for the first page being freed, then leak_balloon() will create a scatter list with len=0. Which in turn seems to generate an invalid virtio request.
I didn't get this in practice, I found it by code review. On the other hand, such an invalid virtio request will cause errors in QEMU and fill_balloon() also performs the same check implemented by this commit.
Signed-off-by Luiz Capitulino <[email protected]>
Acked-by Rafael Aquini <[email protected]>
---
o v2
- Improve changelog
 drivers/virtio/virtio_balloon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/virtio/virtio_balloon.c +0 -0
index bd3ae32..71af7b5 100644

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 194

Quoted 4 lines

drivers/virtio/virtio_balloon.c +0 -1

@@ -191,7 +191,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 	 * is true, we *have* to do it in this order
 	 */
-	tell_host(vb, vb->deflate_vq);

Luiz, sorry for not being clearer before. I was referring to add a commentary on code, to explain in short words why we should not get rid of this check point.

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 196

Quoted 3 lines

drivers/virtio/virtio_balloon.c +2 -0

+	if (vb->num_pfns != 0)
+		tell_host(vb, vb->deflate_vq);
 	mutex_unlock(&vb->balloon_lock);

If the comment is regarded as unnecessary, then just ignore my suggestion. I'm OK with your patch. :)

Cheers!
-- Rafael

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Luiz Capitulino

author

2013-06-06 14:19 UTC #2 lore raw

Quoted 21 lines · On Thu, 6 Jun 2013 11:13:58 -0300 Rafael Aquini <[email protected]> wrote:

Quoted 20 lines · Luiz Capitulino
The balloon_page_dequeue() function can return NULL. If it does for the first page being freed, then leak_balloon() will create a scatter list with len=0. Which in turn seems to generate an invalid virtio request.
I didn't get this in practice, I found it by code review. On the other hand, such an invalid virtio request will cause errors in QEMU and fill_balloon() also performs the same check implemented by this commit.
Signed-off-by Luiz Capitulino <[email protected]>
Acked-by Rafael Aquini <[email protected]>
---
o v2
- Improve changelog
 drivers/virtio/virtio_balloon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 12 lines

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 194
Quoted 9 lines
drivers/virtio/virtio_balloon.c +0 -1
index bd3ae32..71af7b5 100644
@@ -191,7 +191,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 	 * is true, we *have* to do it in this order
 	 */
-	tell_host(vb, vb->deflate_vq);
Luiz, sorry for not being clearer before. I was referring to add a commentary on code, to explain in short words why we should not get rid of this check point.

Oh.

Quoted 6 lines

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 196
Quoted 3 lines
drivers/virtio/virtio_balloon.c +2 -0
+	if (vb->num_pfns != 0)
+		tell_host(vb, vb->deflate_vq);
 	mutex_unlock(&vb->balloon_lock);
If the comment is regarded as unnecessary, then just ignore my suggestion. I'm OK with your patch. :)

IMHO, the code is clear enough.

Luiz Capitulino

author

2013-06-17 17:17 UTC #3 lore raw

In reply to Luiz Capitulino

Quoted 11 lines · On Wed, 5 Jun 2013 21:18:37 -0400 Luiz Capitulino <[email protected]>…

The balloon_page_dequeue() function can return NULL. If it does for the first page being freed, then leak_balloon() will create a scatter list with len=0. Which in turn seems to generate an invalid virtio request.
I didn't get this in practice, I found it by code review. On the other hand, such an invalid virtio request will cause errors in QEMU and fill_balloon() also performs the same check implemented by this commit.
Signed-off-by Luiz Capitulino <[email protected]>
Acked-by Rafael Aquini <[email protected]>

Andrew, can you pick this one?

Quoted 14 lines

---
o v2
- Improve changelog
 drivers/virtio/virtio_balloon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/virtio/virtio_balloon.c +0 -0
index bd3ae32..71af7b5 100644

drivers/virtio/virtio_balloon.c static void leak_balloon(struct virtio_balloon *vb, size_t num) · line 198

Quoted 9 lines

drivers/virtio/virtio_balloon.c +2 -1

@@ -191,7 +191,8 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
 	 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 	 * is true, we *have* to do it in this order
 	 */
-	tell_host(vb, vb->deflate_vq);
+	if (vb->num_pfns != 0)
+		tell_host(vb, vb->deflate_vq);
 	mutex_unlock(&vb->balloon_lock);
 	release_pages_by_pfn(vb->pfns, vb->num_pfns);
 }