virtio-serial: Trivial fixes, don't copy buffers to host

PATCH New Amit Shah 1 participant 5 messages Jan 11, 2011

lore

Share & tools

lore

Amit Shah

author

2011-01-11 09:33 UTC #0 lore raw

v3 cover virtio-serial: Trivial fixes, don't copy buffers to host

Hi,

This patch series converts virtio-serial-bus to use the guest buffers instead of copying over guest data to the host, as suggested by Paul.

In addition, there are some trivial fixes and code re-arrangement to the virtio-console and virtio-serial code for the upcoming flow control series.

Please apply.

v3:
- Move discard logic to a separate function
- Drop while-loop-rework patch as it's now no longer necessary with the new discard logic

v2:
- drop the erroring out patch till we decide what's to be done
- remove goto usage.

Amit Shah (4):
virtio-console: Factor out common init between console and generic
ports
virtio-console: Remove unnecessary braces
virtio-serial-bus: separate out discard logic in a separate function
virtio-serial: Don't copy over guest buffer to host

 hw/virtio-console.c    |   34 ++++++++++++++------------------
 hw/virtio-serial-bus.c |   50 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 47 insertions(+), 37 deletions(-)

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-11 09:33 UTC #1 lore raw

PATCH v3 1/4 virtio-console: Factor out common init between console and generic ports

The initialisation for generic ports and console ports is similar. Factor out the parts that are the same in a different function that can be called from each of the initfns.

Signed-off-by Amit Shah <[email protected]>

---
 hw/virtio-console.c |   31 ++++++++++++++-----------------
 1 files changed, 14 insertions(+), 17 deletions(-)

hw/virtio-console.c +14 ~~-17~~

index caea11f..d7fe68b 100644
@@ -58,24 +58,28 @@ static void chr_event(void *opaque, int event)
     }
 }
 
-/* Virtio Console Ports */
-static int virtconsole_initfn(VirtIOSerialDevice *dev)
+static int generic_port_init(VirtConsole *vcon, VirtIOSerialDevice *dev)
 {
-    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    port->info = dev->info;
-
-    port->is_console = true;
+    vcon->port.info = dev->info;
 
     if (vcon->chr) {
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
-        port->info->have_data = flush_buf;
+        vcon->port.info->have_data = flush_buf;
     }
     return 0;
 }
 
+/* Virtio Console Ports */
+static int virtconsole_initfn(VirtIOSerialDevice *dev)
+{
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    port->is_console = true;
+    return generic_port_init(vcon, dev);
+}
+
 static int virtconsole_exitfn(VirtIOSerialDevice *dev)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);

@@ -115,14 +119,7 @@ static int virtserialport_initfn(VirtIOSerialDevice *dev)
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
-    port->info = dev->info;
-
-    if (vcon->chr) {
-        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
-                              vcon);
-        port->info->have_data = flush_buf;
-    }
-    return 0;
+    return generic_port_init(vcon, dev);
 }
 
 static VirtIOSerialPortInfo virtserialport_info = {

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-11 09:33 UTC #2 lore raw

PATCH v3 2/4 virtio-console: Remove unnecessary braces

In reply to Amit Shah

Remove unnecessary braces around a case statement.

Signed-off-by Amit Shah <[email protected]>

---
 hw/virtio-console.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

hw/virtio-console.c +1 -2

index d7fe68b..d0b9354 100644
@@ -48,10 +48,9 @@ static void chr_event(void *opaque, int event)
     VirtConsole *vcon = opaque;
 
     switch (event) {
-    case CHR_EVENT_OPENED: {
+    case CHR_EVENT_OPENED:
         virtio_serial_open(&vcon->port);
         break;
-    }
     case CHR_EVENT_CLOSED:
         virtio_serial_close(&vcon->port);
         break;

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-11 09:33 UTC #3 lore raw

PATCH v3 3/4 virtio-serial-bus: separate out discard logic in a separate function

In reply to Amit Shah

Instead of combining flush logic into the discard case and not discard case, have one function doing discard case. This will help later when adding flow control logic to the do_flush_queued_data() function.

Signed-off-by Amit Shah <[email protected]>

---
 hw/virtio-serial-bus.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

hw/virtio-serial-bus.c +30 ~~-17~~

index 74ba5ec..e8c2a16 100644
@@ -113,39 +113,48 @@ static size_t write_to_port(VirtIOSerialPort *port,
     return offset;
 }
 
+static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
+{
+    VirtQueueElement elem;
+
+    while (virtqueue_pop(vq, &elem)) {
+        virtqueue_push(vq, &elem, 0);
+    }
+    virtio_notify(vdev, vq);
+}
+
 static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
-                                 VirtIODevice *vdev, bool discard)
+                                 VirtIODevice *vdev)
 {
     VirtQueueElement elem;
 
-    assert(port || discard);
+    assert(port);
     assert(virtio_queue_ready(vq));
 
-    while ((discard || !port->throttled) && virtqueue_pop(vq, &elem)) {
+    while (!port->throttled && virtqueue_pop(vq, &elem)) {
         uint8_t *buf;
         size_t ret, buf_size;
 
-        if (!discard) {
-            buf_size = iov_size(elem.out_sg, elem.out_num);
-            buf = qemu_malloc(buf_size);
-            ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+        buf_size = iov_size(elem.out_sg, elem.out_num);
+        buf = qemu_malloc(buf_size);
+        ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+
+        port->info->have_data(port, buf, ret);
+        qemu_free(buf);
 
-            port->info->have_data(port, buf, ret);
-            qemu_free(buf);
-        }
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);
 }
 
-static void flush_queued_data(VirtIOSerialPort *port, bool discard)
+static void flush_queued_data(VirtIOSerialPort *port)
 {
     assert(port);
 
     if (!virtio_queue_ready(port->ovq)) {
         return;
     }
-    do_flush_queued_data(port, port->ovq, &port->vser->vdev, discard);
+    do_flush_queued_data(port, port->ovq, &port->vser->vdev);
 }
 
 static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)

@@ -204,7 +213,7 @@ int virtio_serial_close(VirtIOSerialPort *port)
      * consume, reset the throttling flag and discard the data.
      */
     port->throttled = false;
-    flush_queued_data(port, true);
+    discard_vq_data(port->ovq, &port->vser->vdev);
 
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);

@@ -258,7 +267,7 @@ void virtio_serial_throttle_port(VirtIOSerialPort *port, bool throttle)
         return;
     }
 
-    flush_queued_data(port, false);
+    flush_queued_data(port);
 }
 
 /* Guest wants to notify us of some event */

@@ -414,11 +423,15 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
         discard = true;
     }
 
-    if (!discard && port->throttled) {
+    if (discard) {
+        discard_vq_data(vq, vdev);
+        return;
+    }
+    if (port->throttled) {
         return;
     }
 
-    do_flush_queued_data(port, vq, vdev, discard);
+    do_flush_queued_data(port, vq, vdev);
 }
 
 static void handle_input(VirtIODevice *vdev, VirtQueue *vq)

@@ -634,7 +647,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
 
     port = find_port_by_id(vser, port_id);
     /* Flush out any unconsumed buffers first */
-    flush_queued_data(port, true);
+    discard_vq_data(port->ovq, &port->vser->vdev);
 
     send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
 }

Signature · 1 line

1.7.3.4

Amit Shah

author

2011-01-11 09:33 UTC #4 lore raw

PATCH v3 4/4 virtio-serial: Don't copy over guest buffer to host

In reply to Amit Shah

When the guest writes something to a host, we copied over the entire buffer first into the host and then processed it. Do away with that, it could result in a malicious guest causing a DoS on the host.

Reported-by Paul Brook <[email protected]>

Signed-off-by Amit Shah <[email protected]>

---
 hw/virtio-serial-bus.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

hw/virtio-serial-bus.c +8 -7

index e8c2a16..6726f72 100644
@@ -132,16 +132,17 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
     assert(virtio_queue_ready(vq));
 
     while (!port->throttled && virtqueue_pop(vq, &elem)) {
-        uint8_t *buf;
-        size_t ret, buf_size;
+        unsigned int i;
 
-        buf_size = iov_size(elem.out_sg, elem.out_num);
-        buf = qemu_malloc(buf_size);
-        ret = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, buf_size);
+        for (i = 0; i < elem.out_num; i++) {
+            size_t buf_size;
 
-        port->info->have_data(port, buf, ret);
-        qemu_free(buf);
+            buf_size = elem.out_sg[i].iov_len;
 
+            port->info->have_data(port,
+                                  elem.out_sg[i].iov_base,
+                                  buf_size);
+        }
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);

Signature · 1 line

1.7.3.4