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

PATCH Superseded Luiz Capitulino 2 participants 4 messages Jun 5, 2013

Subsystems (4)

Memory management KVM virtio core virtio balloon

lore

Share & tools

lore

Luiz Capitulino

author

2013-06-05 21:10 UTC #0 lore raw

PATCH 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.

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

---

PS: I didn't get this in practice. I found it by code review. On the other
hand, automatic-ballooning was able to put such invalid requests in the virtqueue and QEMU would explode...

PPS: Very lightly tested

 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

Acked-by on v1 1/1

2013-06-05 21:24 UTC #1 lore raw

Quoted 5 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.

Quoted 7 lines

Signed-off-by Luiz Capitulino <[email protected]>
---
PS: I didn't get this in practice. I found it by code review. On the other
hand, automatic-ballooning was able to put such invalid requests in the virtqueue and QEMU would explode...

Nice catch! The patch looks sane and replicates the check done at fill_balloon(). I think we also could use this P.S. as a commentary to let others aware of this scenario. Thanks Luiz!

Acked-by Rafael Aquini <[email protected]>

Quoted 22 lines

PPS: Very lightly tested

 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 · 1 line

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>

Luiz Capitulino

author

2013-06-05 23:08 UTC #2 lore raw

Quoted 9 lines · On Wed, 5 Jun 2013 18:24:49 -0300 Rafael Aquini <[email protected]> wrote:

Quoted 8 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.
Signed-off-by Luiz Capitulino <[email protected]>
---

Quoted 8 lines

Quoted 4 lines
PS: I didn't get this in practice. I found it by code review. On the other
hand, automatic-ballooning was able to put such invalid requests in the virtqueue and QEMU would explode...
Nice catch! The patch looks sane and replicates the check done at fill_balloon(). I think we also could use this P.S. as a commentary to let others aware of this scenario. Thanks Luiz!

Want me to respin?

Quoted 1 line

Acked-by Rafael Aquini <[email protected]>

Thanks for your review!

Quoted 16 lines

PPS: Very lightly tested

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

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);

Quoted 9 lines

Quoted 8 lines

drivers/virtio/virtio_balloon.c +2 -0

+	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 · 1 line

1.8.1.4

Rafael Aquini

2013-06-05 23:29 UTC #3 lore raw

Quoted 17 lines · Luiz Capitulino

Quoted 14 lines · On Wed, 5 Jun 2013 18:24:49 -0300 Rafael Aquini <[email protected]> wrote:
Quoted 12 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.
Signed-off-by Luiz Capitulino <[email protected]>
---
PS: I didn't get this in practice. I found it by code review. On the other
hand, automatic-ballooning was able to put such invalid requests in the virtqueue and QEMU would explode...

Quoted 6 lines

Quoted 3 lines
Nice catch! The patch looks sane and replicates the check done at fill_balloon(). I think we also could use this P.S. as a commentary to let others aware of this scenario. Thanks Luiz!
Want me to respin?

That would be great, indeed. I guess the commentary could also go for the same if case at fill_balloon(), assuming the tests are placed to prevent the same scenario you described at changelog. You can stick my Ack on it, if reposting.

Cheers!
-- Rafael