[Qemu-devel] [PATCH 0/4] virtio-net: do not leak cpu mappings

[Qemu-devel] [PATCH 0/4] virtio-net: do not leak cpu mappings

[Stefano Stabellini]

Hi all,
this patch series fixes a cpu mapping leak in virtio-net.

The bug is caused by virtio_net_handle_ctrl: it maps the entire out_sg
iov, but then modifies it and reduces it (iov_discard_front), and only
unmap the reduced version of the iov.

This causes a crash when running on Xen, but the behaviour is obviously
incorrect without Xen too.

The patch series fixes the issue by allowing virtio_net_handle_ctrl to
unmap the original out_sg iov but still call virtqueue_fill and
virtqueue_flush on the modified iov.

The first three patches do not introduce any functional changes.


Stefano Stabellini (4):
      introduce virtqueue_unmap_sg
      use virtqueue_unmap_sg in virtqueue_fill
      move virtqueue_unmap_sg calls from virtqueue_fill to virtqueue_push
      virtio-net: do not leak cpu mappings

 hw/net/virtio-net.c        |    9 ++++++++-
 hw/virtio/virtio.c         |   43 ++++++++++++++++++++++++-------------------
 include/hw/virtio/virtio.h |    2 ++
 3 files changed, 34 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH 1/4] introduce virtqueue_unmap_sg

[Stefano Stabellini]

Introduce a function to unmap an sg previously mapped with
virtqueue_map_sg.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: jasowang@redhat.com
CC: wency@cn.fujitsu.com
CC: mst@redhat.com
CC: pbonzini@redhat.com
---
 hw/virtio/virtio.c         |   22 ++++++++++++++++++++++
 include/hw/virtio/virtio.h |    2 ++
 2 files changed, 24 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3e4b70c..2621ae6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -446,6 +446,28 @@ void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
     }
 }
 
+void virtqueue_unmap_sg(const struct iovec *sg, size_t num_sg,
+                        int is_write, unsigned int len)
+{
+    unsigned int i;
+    unsigned int offset;
+
+    if (num_sg > VIRTQUEUE_MAX_SIZE) {
+        error_report("virtio: unmap attempt out of bounds: %zd > %d",
+                     num_sg, VIRTQUEUE_MAX_SIZE);
+        exit(1);
+    }
+
+    offset = 0;
+    for (i = 0; i < num_sg; i++) {
+        size_t size = MIN(len - offset, sg[i].iov_len);
+
+        cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len, is_write, size);
+
+        offset += size;
+    }
+}
+
 int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 {
     unsigned int i, head, max;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3e54e90..2325053 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -173,6 +173,8 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 
 void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
     size_t num_sg, int is_write);
+void virtqueue_unmap_sg(const struct iovec *sg, size_t num_sg,
+                        int is_write, unsigned int len);
 int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
 int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
                           unsigned int out_bytes);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/4] introduce virtqueue_unmap_sg

[Peter Maydell]

On 25 November 2014 at 14:43, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> Introduce a function to unmap an sg previously mapped with
> virtqueue_map_sg.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: jasowang@redhat.com
> CC: wency@cn.fujitsu.com
> CC: mst@redhat.com
> CC: pbonzini@redhat.com
> ---
>  hw/virtio/virtio.c         |   22 ++++++++++++++++++++++
>  include/hw/virtio/virtio.h |    2 ++
>  2 files changed, 24 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3e4b70c..2621ae6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -446,6 +446,28 @@ void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
>      }
>  }
>
> +void virtqueue_unmap_sg(const struct iovec *sg, size_t num_sg,
> +                        int is_write, unsigned int len)
> +{
> +    unsigned int i;
> +    unsigned int offset;
> +
> +    if (num_sg > VIRTQUEUE_MAX_SIZE) {
> +        error_report("virtio: unmap attempt out of bounds: %zd > %d",
> +                     num_sg, VIRTQUEUE_MAX_SIZE);
> +        exit(1);
> +    }
> +
> +    offset = 0;
> +    for (i = 0; i < num_sg; i++) {
> +        size_t size = MIN(len - offset, sg[i].iov_len);
> +
> +        cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len, is_write, size);
> +
> +        offset += size;
> +    }
> +}

It seems rather odd that "size" and the iov_len fields in
the iovec are size_t but 'len' and 'offset' are only
unsigned int...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] introduce virtqueue_unmap_sg

[Stefano Stabellini]

On Tue, 25 Nov 2014, Peter Maydell wrote:
> On 25 November 2014 at 14:43, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > Introduce a function to unmap an sg previously mapped with
> > virtqueue_map_sg.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: jasowang@redhat.com
> > CC: wency@cn.fujitsu.com
> > CC: mst@redhat.com
> > CC: pbonzini@redhat.com
> > ---
> >  hw/virtio/virtio.c         |   22 ++++++++++++++++++++++
> >  include/hw/virtio/virtio.h |    2 ++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 3e4b70c..2621ae6 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -446,6 +446,28 @@ void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> >      }
> >  }
> >
> > +void virtqueue_unmap_sg(const struct iovec *sg, size_t num_sg,
> > +                        int is_write, unsigned int len)
> > +{
> > +    unsigned int i;
> > +    unsigned int offset;
> > +
> > +    if (num_sg > VIRTQUEUE_MAX_SIZE) {
> > +        error_report("virtio: unmap attempt out of bounds: %zd > %d",
> > +                     num_sg, VIRTQUEUE_MAX_SIZE);
> > +        exit(1);
> > +    }
> > +
> > +    offset = 0;
> > +    for (i = 0; i < num_sg; i++) {
> > +        size_t size = MIN(len - offset, sg[i].iov_len);
> > +
> > +        cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len, is_write, size);
> > +
> > +        offset += size;
> > +    }
> > +}
> 
> It seems rather odd that "size" and the iov_len fields in
> the iovec are size_t but 'len' and 'offset' are only
> unsigned int...

The code is taken from virtqueue_fill but that's a good point.

[Qemu-devel] [PATCH 2/4] use virtqueue_unmap_sg in virtqueue_fill

[Stefano Stabellini]

Use virtqueue_unmap_sg to unmap in_sg and out_sg in virtqueue_fill.

No functional changes.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: jasowang@redhat.com
CC: wency@cn.fujitsu.com
CC: mst@redhat.com
CC: pbonzini@redhat.com
---
 hw/virtio/virtio.c |   20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2621ae6..4af31d0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -238,26 +238,10 @@ int virtio_queue_empty(VirtQueue *vq)
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx)
 {
-    unsigned int offset;
-    int i;
-
     trace_virtqueue_fill(vq, elem, len, idx);
 
-    offset = 0;
-    for (i = 0; i < elem->in_num; i++) {
-        size_t size = MIN(len - offset, elem->in_sg[i].iov_len);
-
-        cpu_physical_memory_unmap(elem->in_sg[i].iov_base,
-                                  elem->in_sg[i].iov_len,
-                                  1, size);
-
-        offset += size;
-    }
-
-    for (i = 0; i < elem->out_num; i++)
-        cpu_physical_memory_unmap(elem->out_sg[i].iov_base,
-                                  elem->out_sg[i].iov_len,
-                                  0, elem->out_sg[i].iov_len);
+    virtqueue_unmap_sg(elem->in_sg, elem->in_num, 1, len);
+    virtqueue_unmap_sg(elem->out_sg, elem->out_num, 0, UINT_MAX);
 
     idx = (idx + vring_used_idx(vq)) % vq->vring.num;
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/4] move virtqueue_unmap_sg calls from virtqueue_fill to virtqueue_push

[Stefano Stabellini]

Move the two virtqueue_unmap_sg calls from virtqueue_fill to
virtqueue_push: most virtio drivers simply call virtqueue_push so they
are unaffected. virtio-net calls virtqueue_fill directly, so we need to
make the two virtqueue_unmap_sg calls explicitely there.

Also replace the virtqueue_push call from virtio_net_handle_ctrl with an
open coded version it in place.

No functional changes.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: jasowang@redhat.com
CC: wency@cn.fujitsu.com
CC: mst@redhat.com
CC: pbonzini@redhat.com
---
 hw/net/virtio-net.c |    7 ++++++-
 hw/virtio/virtio.c  |    5 ++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 2ac6ce5..5035313 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -803,7 +803,10 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         s = iov_from_buf(elem.in_sg, elem.in_num, 0, &status, sizeof(status));
         assert(s == sizeof(status));
 
-        virtqueue_push(vq, &elem, sizeof(status));
+        virtqueue_unmap_sg(elem.in_sg, elem.in_num, 1, sizeof(status));
+        virtqueue_unmap_sg(elem.out_sg, elem.out_num, 0, UINT_MAX);
+        virtqueue_fill(vq, &elem, sizeof(status), 0);
+        virtqueue_flush(vq, 1);
         virtio_notify(vdev, vq);
     }
 }
@@ -1052,6 +1055,8 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
         }
 
         /* signal other side */
+        virtqueue_unmap_sg(elem.in_sg, elem.in_num, 1, total);
+        virtqueue_unmap_sg(elem.out_sg, elem.out_num, 0, UINT_MAX);
         virtqueue_fill(q->rx_vq, &elem, total, i++);
     }
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4af31d0..0a6583a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -240,9 +240,6 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 {
     trace_virtqueue_fill(vq, elem, len, idx);
 
-    virtqueue_unmap_sg(elem->in_sg, elem->in_num, 1, len);
-    virtqueue_unmap_sg(elem->out_sg, elem->out_num, 0, UINT_MAX);
-
     idx = (idx + vring_used_idx(vq)) % vq->vring.num;
 
     /* Get a pointer to the next entry in the used ring. */
@@ -267,6 +264,8 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
 void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len)
 {
+    virtqueue_unmap_sg(elem->in_sg, elem->in_num, 1, len);
+    virtqueue_unmap_sg(elem->out_sg, elem->out_num, 0, UINT_MAX);
     virtqueue_fill(vq, elem, len, 0);
     virtqueue_flush(vq, 1);
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/4] virtio-net: do not leak cpu mappings

[Stefano Stabellini]

In virtio_net_handle_ctrl unmap the previously mapped out_sg, not a
subset of it.

This patch fixes an abort() when running on Xen.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: jasowang@redhat.com
CC: wency@cn.fujitsu.com
CC: mst@redhat.com
CC: pbonzini@redhat.com
---
 hw/net/virtio-net.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5035313..c9b82f3 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -773,6 +773,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     VirtQueueElement elem;
     size_t s;
     struct iovec *iov;
+    struct iovec iov_copy[VIRTQUEUE_MAX_SIZE];
     unsigned int iov_cnt;
 
     while (virtqueue_pop(vq, &elem)) {
@@ -782,6 +783,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             exit(1);
         }
 
+        memcpy(iov_copy, elem.out_sg, elem.out_num*sizeof(struct iovec));
         iov = elem.out_sg;
         iov_cnt = elem.out_num;
         s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
@@ -804,7 +806,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         assert(s == sizeof(status));
 
         virtqueue_unmap_sg(elem.in_sg, elem.in_num, 1, sizeof(status));
-        virtqueue_unmap_sg(elem.out_sg, elem.out_num, 0, UINT_MAX);
+        virtqueue_unmap_sg(iov_copy, elem.out_num, 0, UINT_MAX);
         virtqueue_fill(vq, &elem, sizeof(status), 0);
         virtqueue_flush(vq, 1);
         virtio_notify(vdev, vq);
-- 
1.7.10.4

Re: [Qemu-devel] [Xen-devel] [PATCH 0/4] virtio-net: do not leak cpu mappings

[Fabio Fantoni]

Il 25/11/2014 15:42, Stefano Stabellini ha scritto:
> Hi all,
> this patch series fixes a cpu mapping leak in virtio-net.
>
> The bug is caused by virtio_net_handle_ctrl: it maps the entire out_sg
> iov, but then modifies it and reduces it (iov_discard_front), and only
> unmap the reduced version of the iov.
>
> This causes a crash when running on Xen, but the behaviour is obviously
> incorrect without Xen too.
>
> The patch series fixes the issue by allowing virtio_net_handle_ctrl to
> unmap the original out_sg iov but still call virtqueue_fill and
> virtqueue_flush on the modified iov.
>
> The first three patches do not introduce any functional changes.

Thanks for these pathes, 1-2 years ago I tried to use virtio net and 
disks on xen unsuccessful and I was unable to solve it.
When I'll have time I'll retry.
About virtio disks can have problem similar to this or should be ok?
About the other patches posted in link below are all not applied and 
should be only rebased or they must be fully revisioned/modified?
http://wiki.xen.org/wiki/Virtio_On_Xen

Thanks for any reply and sorry for my bad english.

>
>
> Stefano Stabellini (4):
>        introduce virtqueue_unmap_sg
>        use virtqueue_unmap_sg in virtqueue_fill
>        move virtqueue_unmap_sg calls from virtqueue_fill to virtqueue_push
>        virtio-net: do not leak cpu mappings
>
>   hw/net/virtio-net.c        |    9 ++++++++-
>   hw/virtio/virtio.c         |   43 ++++++++++++++++++++++++-------------------
>   include/hw/virtio/virtio.h |    2 ++
>   3 files changed, 34 insertions(+), 20 deletions(-)
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Qemu-devel] [Xen-devel] [PATCH 0/4] virtio-net: do not leak cpu mappings

[Wei Liu]

On Tue, Nov 25, 2014 at 04:28:32PM +0100, Fabio Fantoni wrote:
> Il 25/11/2014 15:42, Stefano Stabellini ha scritto:
> >Hi all,
> >this patch series fixes a cpu mapping leak in virtio-net.
> >
> >The bug is caused by virtio_net_handle_ctrl: it maps the entire out_sg
> >iov, but then modifies it and reduces it (iov_discard_front), and only
> >unmap the reduced version of the iov.
> >
> >This causes a crash when running on Xen, but the behaviour is obviously
> >incorrect without Xen too.
> >
> >The patch series fixes the issue by allowing virtio_net_handle_ctrl to
> >unmap the original out_sg iov but still call virtqueue_fill and
> >virtqueue_flush on the modified iov.
> >
> >The first three patches do not introduce any functional changes.
> 
> Thanks for these pathes, 1-2 years ago I tried to use virtio net and disks
> on xen unsuccessful and I was unable to solve it.
> When I'll have time I'll retry.
> About virtio disks can have problem similar to this or should be ok?
> About the other patches posted in link below are all not applied and should
> be only rebased or they must be fully revisioned/modified?
> http://wiki.xen.org/wiki/Virtio_On_Xen
> 

Don't use patches on that page. They are outdated.

I think Stefano's patch series intends to fix issue for HVM guest, which
might well be what you're using.  Well, I would be very surprised if you
tell me you're using PV virtio! ;-)

Wei.