[Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[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 <wency@cn.fujitsu.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from V1:
- Use g_memdup() to simplify codes
---
 hw/net/virtio-net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9b88775..e574bd4 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -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);
     }
 }
 
-- 
1.9.1

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Stefano Stabellini]

On Thu, 27 Nov 2014, Jason Wang wrote:
> 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 <wency@cn.fujitsu.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> Changes from V1:
> - Use g_memdup() to simplify codes
> ---
>  hw/net/virtio-net.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9b88775..e574bd4 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -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);
>      }
>  }
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Fam Zheng]

On Thu, 11/27 18:04, Jason Wang wrote:
> 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 <wency@cn.fujitsu.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

> ---
> Changes from V1:
> - Use g_memdup() to simplify codes
> ---
>  hw/net/virtio-net.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9b88775..e574bd4 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -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);
>      }
>  }
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Michael S. Tsirkin]

On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
> 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 <wency@cn.fujitsu.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

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

> ---
> Changes from V1:
> - Use g_memdup() to simplify codes
> ---
>  hw/net/virtio-net.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 9b88775..e574bd4 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -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);
>      }
>  }
>  
> -- 
> 1.9.1

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Peter Maydell]

On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
>> 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 <wency@cn.fujitsu.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> 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

Re: [Qemu-devel] [2.2 PATCH V2 for-4.5] virtio-net: fix unmap leak

[Stefano Stabellini]

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


On Thu, 27 Nov 2014, Peter Maydell wrote:
> On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
> >> 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 <wency@cn.fujitsu.com>
> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > 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...

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Peter Maydell]

On 27 November 2014 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
>>> 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 <wency@cn.fujitsu.com>
>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> 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

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Michael S. Tsirkin]

On Thu, Nov 27, 2014 at 02:09:06PM +0000, Peter Maydell wrote:
> On 27 November 2014 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
> >>> 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 <wency@cn.fujitsu.com>
> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Cc: qemu-stable@nongnu.org
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> 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

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

-- 
MSR

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Stefano Stabellini]

On Thu, 27 Nov 2014, Peter Maydell wrote:
> On 27 November 2014 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
> >>> 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 <wency@cn.fujitsu.com>
> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Cc: qemu-stable@nongnu.org
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> 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.

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.

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Michael S. Tsirkin]

On Thu, Nov 27, 2014 at 02:54:47PM +0000, Stefano Stabellini wrote:
> On Thu, 27 Nov 2014, Peter Maydell wrote:
> > On 27 November 2014 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
> > >>> 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 <wency@cn.fujitsu.com>
> > >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >>> Cc: qemu-stable@nongnu.org
> > >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >>
> > >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > >>
> > >> 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.
> 
> 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.

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

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Michael S. Tsirkin]

On Thu, Nov 27, 2014 at 02:09:06PM +0000, Peter Maydell wrote:
> On 27 November 2014 at 12:42, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 27 November 2014 at 12:33, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Thu, Nov 27, 2014 at 06:04:03PM +0800, Jason Wang wrote:
> >>> 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 <wency@cn.fujitsu.com>
> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Cc: qemu-stable@nongnu.org
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>
> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> 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

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

-- 
MST

Re: [Qemu-devel] [2.2 PATCH V2] virtio-net: fix unmap leak

[Peter Maydell]

On 27 November 2014 at 10:04, Jason Wang <jasowang@redhat.com> wrote:
> 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 <wency@cn.fujitsu.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> 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