virtio-net: fix unmap leak

PATCH Applied Jason Wang 5 participants 12 messages Nov 27, 2014 – Nov 28, 2014

Share & tools

Jason Wang

author

2014-11-27 10:04 UTC #0 lore raw

PATCH v2 1/1 virtio-net: fix unmap leak

virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.

Fixes this by keeping the original iov untouched and using a temp variable in those functions.

Cc Wen Congyang <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc [email protected]

Signed-off-by Jason Wang <[email protected]>

---
Changes from V1:
- Use g_memdup() to simplify codes

---
 hw/net/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

hw/net/virtio-net.c +3 -2

index 9b88775..e574bd4 100644
@@ -798,7 +798,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     VirtQueueElement elem;
     size_t s;
-    struct iovec *iov;
+    struct iovec *iov, *iov2;
     unsigned int iov_cnt;
 
     while (virtqueue_pop(vq, &elem)) {

@@ -808,8 +808,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             exit(1);
         }
 
-        iov = elem.out_sg;
         iov_cnt = elem.out_num;
+        iov2 = iov = g_memdup(elem.out_sg, sizeof(struct iovec) * elem.out_num);
         s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
         iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
         if (s != sizeof(ctrl)) {

@@ -833,6 +833,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
         virtqueue_push(vq, &elem, sizeof(status));
         virtio_notify(vdev, vq);
+        g_free(iov2);
     }
 }

Signature · 1 line

1.9.1

Stefano Stabellini

Reviewed-by on v2 1/1

2014-11-27 10:28 UTC #1 lore raw

Quoted 11 lines · Jason Wang

virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>

Reviewed-by Stefano Stabellini <[email protected]>

Quoted 40 lines

Changes from V1:
- Use g_memdup() to simplify codes

---
 hw/net/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

hw/net/virtio-net.c +3 -2

index 9b88775..e574bd4 100644
@@ -798,7 +798,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     VirtQueueElement elem;
     size_t s;
-    struct iovec *iov;
+    struct iovec *iov, *iov2;
     unsigned int iov_cnt;
 
     while (virtqueue_pop(vq, &elem)) {

@@ -808,8 +808,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             exit(1);
         }
 
-        iov = elem.out_sg;
         iov_cnt = elem.out_num;
+        iov2 = iov = g_memdup(elem.out_sg, sizeof(struct iovec) * elem.out_num);
         s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
         iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
         if (s != sizeof(ctrl)) {

@@ -833,6 +833,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
         virtqueue_push(vq, &elem, sizeof(status));
         virtio_notify(vdev, vq);
+        g_free(iov2);
     }
 }

Signature · 1 line

1.9.1

Fam Zheng

Reviewed-by on v2 1/1

2014-11-27 10:30 UTC #2 lore raw

In reply to Jason Wang

Quoted 11 lines · Jason Wang

virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Quoted 41 lines

---
Changes from V1:
- Use g_memdup() to simplify codes

---
 hw/net/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

hw/net/virtio-net.c +3 -2

index 9b88775..e574bd4 100644
@@ -798,7 +798,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     VirtQueueElement elem;
     size_t s;
-    struct iovec *iov;
+    struct iovec *iov, *iov2;
     unsigned int iov_cnt;
 
     while (virtqueue_pop(vq, &elem)) {

@@ -808,8 +808,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             exit(1);
         }
 
-        iov = elem.out_sg;
         iov_cnt = elem.out_num;
+        iov2 = iov = g_memdup(elem.out_sg, sizeof(struct iovec) * elem.out_num);
         s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
         iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
         if (s != sizeof(ctrl)) {

@@ -833,6 +833,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
         virtqueue_push(vq, &elem, sizeof(status));
         virtio_notify(vdev, vq);
+        g_free(iov2);
     }
 }

Signature · 1 line

1.9.1

Michael S. Tsirkin

Reviewed-by on v2 1/1

2014-11-27 12:34 UTC #3 lore raw

In reply to Jason Wang

Quoted 11 lines · Jason Wang

virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>

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

Peter, can you pick this up or do you want a pull request?

Quoted 40 lines

---
Changes from V1:
- Use g_memdup() to simplify codes

---
 hw/net/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

hw/net/virtio-net.c +3 -2

index 9b88775..e574bd4 100644
@@ -798,7 +798,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     VirtQueueElement elem;
     size_t s;
-    struct iovec *iov;
+    struct iovec *iov, *iov2;
     unsigned int iov_cnt;
 
     while (virtqueue_pop(vq, &elem)) {

@@ -808,8 +808,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             exit(1);
         }
 
-        iov = elem.out_sg;
         iov_cnt = elem.out_num;
+        iov2 = iov = g_memdup(elem.out_sg, sizeof(struct iovec) * elem.out_num);
         s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
         iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
         if (s != sizeof(ctrl)) {

@@ -833,6 +833,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
         virtqueue_push(vq, &elem, sizeof(status));
         virtio_notify(vdev, vq);
+        g_free(iov2);
     }
 }

Signature · 1 line

1.9.1

Peter Maydell

2014-11-27 12:42 UTC #4 lore raw

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Jason Wang
virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.

Quoted 8 lines

Quoted 4 lines
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>
Reviewed-by Michael S. Tsirkin <[email protected]>
Peter, can you pick this up or do you want a pull request?

I can pick it up. I was waiting a bit to check that everybody was happy that this is the correct way to fix the bug and the patch is ok...

-- PMM

Stefano Stabellini

2014-11-27 12:46 UTC #5 lore raw

Konrad, I think we should have this fix in 4.5: without it vif=[ 'model=virtio-net' ] crashes QEMU.

Quoted 21 lines · Peter Maydell

Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Jason Wang
virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Quoted 8 lines
Quoted 4 lines
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>
Reviewed-by Michael S. Tsirkin <[email protected]>
Peter, can you pick this up or do you want a pull request?
I can pick it up. I was waiting a bit to check that everybody was happy that this is the correct way to fix the bug and the patch is ok...

Peter Maydell

2014-11-27 14:09 UTC #6 lore raw

In reply to Peter Maydell

Quoted 14 lines · Peter Maydell

Quoted 13 lines · Michael S. Tsirkin
Quoted 11 lines · Jason Wang
virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>

Quoted 7 lines

Quoted 3 lines
Reviewed-by Michael S. Tsirkin <[email protected]>
Peter, can you pick this up or do you want a pull request?
I can pick it up. I was waiting a bit to check that everybody was happy that this is the correct way to fix the bug and the patch is ok...

...but discussing this with Stefan H on IRC we realised that the same issue also (at least potentially) affects virtio-blk, which suggests that we should fix this by making the core virtio code cope with backends which modify the sglists.

-- PMM

Michael S. Tsirkin

2014-11-27 14:45 UTC #7 lore raw

Quoted 23 lines · Peter Maydell

Quoted 14 lines · Peter Maydell
Quoted 13 lines · Michael S. Tsirkin
Quoted 11 lines · Jason Wang
virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>
Quoted 7 lines
Quoted 3 lines
Reviewed-by Michael S. Tsirkin <[email protected]>
Peter, can you pick this up or do you want a pull request?
I can pick it up. I was waiting a bit to check that everybody was happy that this is the correct way to fix the bug and the patch is ok...

Quoted 6 lines

...but discussing this with Stefan H on IRC we realised that the same issue also (at least potentially) affects virtio-blk, which suggests that we should fix this by making the core virtio code cope with backends which modify the sglists.
-- PMM

Okay, but this does not sound like 2.2 material to me.

Signature · 1 line

MSR

Stefano Stabellini

2014-11-27 14:55 UTC #8 lore raw

In reply to Peter Maydell

Quoted 19 lines · Peter Maydell

Quoted 18 lines · Peter Maydell
Quoted 16 lines · Michael S. Tsirkin
Quoted 11 lines · Jason Wang
virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>
Reviewed-by Michael S. Tsirkin <[email protected]>
Peter, can you pick this up or do you want a pull request?

Quoted 8 lines

Quoted 3 lines
I can pick it up. I was waiting a bit to check that everybody was happy that this is the correct way to fix the bug and the patch is ok...
...but discussing this with Stefan H on IRC we realised that the same issue also (at least potentially) affects virtio-blk, which suggests that we should fix this by making the core virtio code cope with backends which modify the sglists.

I think that a similar patch could be produced against hw/block/virtio-blk.c:virtio_blk_handle_request

Alternatively my series would help by introducing virtqueue_unmap_sg and moving it out of virtqueue_fill. We could call virtqueue_unmap_sg from the drivers (instead of calling it from virtqueue_push) and that would solve the issue.

Michael S. Tsirkin

2014-11-27 15:04 UTC #9 lore raw

Quoted 24 lines · Stefano Stabellini

Quoted 23 lines · Peter Maydell
Quoted 21 lines · Peter Maydell
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Jason Wang
virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Quoted 8 lines
Quoted 4 lines
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>
Reviewed-by Michael S. Tsirkin <[email protected]>
Peter, can you pick this up or do you want a pull request?
I can pick it up. I was waiting a bit to check that everybody was happy that this is the correct way to fix the bug and the patch is ok...

Quoted 7 lines

Quoted 4 lines
...but discussing this with Stefan H on IRC we realised that the same issue also (at least potentially) affects virtio-blk, which suggests that we should fix this by making the core virtio code cope with backends which modify the sglists.
I think that a similar patch could be produced against hw/block/virtio-blk.c:virtio_blk_handle_request

Hmm, this is a data path operation. Adding malloc/free calls there will slow things down measureably.

Quoted 4 lines

Alternatively my series would help by introducing virtqueue_unmap_sg and moving it out of virtqueue_fill. We could call virtqueue_unmap_sg from the drivers (instead of calling it from virtqueue_push) and that would solve the issue.

Michael S. Tsirkin

2014-11-27 16:40 UTC #10 lore raw

In reply to Peter Maydell

Quoted 23 lines · Peter Maydell

Quoted 14 lines · Peter Maydell
Quoted 13 lines · Michael S. Tsirkin
Quoted 11 lines · Jason Wang
virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>
Quoted 7 lines
Quoted 3 lines
Reviewed-by Michael S. Tsirkin <[email protected]>
Peter, can you pick this up or do you want a pull request?
I can pick it up. I was waiting a bit to check that everybody was happy that this is the correct way to fix the bug and the patch is ok...

Quoted 6 lines

...but discussing this with Stefan H on IRC we realised that the same issue also (at least potentially) affects virtio-blk, which suggests that we should fix this by making the core virtio code cope with backends which modify the sglists.
-- PMM

I just sent a minimal fix for virtio blk.
This way core rework can wait for 2.3.

Signature · 1 line

MST

Peter Maydell

Applied

2014-11-28 13:06 UTC #11 lore raw

In reply to Jason Wang

Quoted 11 lines · Jason Wang

virtio_net_handle_ctrl() and other functions that process control vq request call iov_discard_front() which will shorten the iov. This will lead unmapping in virtqueue_push() leaks mapping.
Fixes this by keeping the original iov untouched and using a temp variable in those functions.
Cc Wen Congyang <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc [email protected]
Signed-off-by Jason Wang <[email protected]>

Quoted 6 lines

---
Changes from V1:
- Use g_memdup() to simplify codes
---
 hw/net/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Applied to master, thanks.

-- PMM