diff mbox

[QEMU,v5,1/3] virtio-net: remove layout assumptions for ctrl vq

Message ID 1358869486-22777-2-git-send-email-akong@redhat.com
State New
Headers show

Commit Message

Amos Kong Jan. 22, 2013, 3:44 p.m. UTC 
From: Michael S. Tsirkin <mst@redhat.com>

Virtio-net code makes assumption about virtqueue descriptor layout
(e.g. sg[0] is the header, sg[1] is the data buffer).

This patch makes code not rely on the layout of descriptors.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
 hw/virtio-net.c |  129 ++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 75 insertions(+), 54 deletions(-)

Comments

Michael S. Tsirkin Jan. 23, 2013, 3:38 p.m. UTC | #1 
On Tue, Jan 22, 2013 at 11:44:44PM +0800, Amos Kong wrote:
> From: Michael S. Tsirkin <mst@redhat.com>
> 
> Virtio-net code makes assumption about virtqueue descriptor layout
> (e.g. sg[0] is the header, sg[1] is the data buffer).
> 
> This patch makes code not rely on the layout of descriptors.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>

Applied all three, thanks.

> ---
>  hw/virtio-net.c |  129 ++++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 75 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 3bb01b1..af1f3a1 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -315,44 +315,44 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>  }
>  
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> -                                     VirtQueueElement *elem)
> +                                     struct iovec *iov, unsigned int iov_cnt)
>  {
>      uint8_t on;
> +    size_t s;
>  
> -    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(on)) {
> -        error_report("virtio-net ctrl invalid rx mode command");
> -        exit(1);
> +    s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
> +    if (s != sizeof(on)) {
> +        return VIRTIO_NET_ERR;
>      }
>  
> -    on = ldub_p(elem->out_sg[1].iov_base);
> -
> -    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC)
> +    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC) {
>          n->promisc = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI) {
>          n->allmulti = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI) {
>          n->alluni = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI) {
>          n->nomulti = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI) {
>          n->nouni = on;
> -    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
> +    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST) {
>          n->nobcast = on;
> -    else
> +    } else {
>          return VIRTIO_NET_ERR;
> +    }
>  
>      return VIRTIO_NET_OK;
>  }
>  
>  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
> -                                 VirtQueueElement *elem)
> +                                 struct iovec *iov, unsigned int iov_cnt)
>  {
>      struct virtio_net_ctrl_mac mac_data;
> +    size_t s;
>  
> -    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
> -        elem->out_sg[1].iov_len < sizeof(mac_data) ||
> -        elem->out_sg[2].iov_len < sizeof(mac_data))
> +    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET) {
>          return VIRTIO_NET_ERR;
> +    }
>  
>      n->mac_table.in_use = 0;
>      n->mac_table.first_multi = 0;
> @@ -360,54 +360,72 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      n->mac_table.multi_overflow = 0;
>      memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>  
> -    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
> +    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
> +                   sizeof(mac_data.entries));
> +    mac_data.entries = ldl_p(&mac_data.entries);
> +    if (s != sizeof(mac_data.entries)) {
> +        return VIRTIO_NET_ERR;
> +    }
> +    iov_discard_front(&iov, &iov_cnt, s);
>  
> -    if (sizeof(mac_data.entries) +
> -        (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
> +    if (mac_data.entries * ETH_ALEN > iov_size(iov, iov_cnt)) {
>          return VIRTIO_NET_ERR;
> +    }
>  
>      if (mac_data.entries <= MAC_TABLE_ENTRIES) {
> -        memcpy(n->mac_table.macs, elem->out_sg[1].iov_base + sizeof(mac_data),
> -               mac_data.entries * ETH_ALEN);
> +        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
> +                       mac_data.entries * ETH_ALEN);
> +        if (s != mac_data.entries * ETH_ALEN) {
> +            return VIRTIO_NET_ERR;
> +        }
>          n->mac_table.in_use += mac_data.entries;
>      } else {
>          n->mac_table.uni_overflow = 1;
>      }
>  
> +    iov_discard_front(&iov, &iov_cnt, mac_data.entries * ETH_ALEN);
> +
>      n->mac_table.first_multi = n->mac_table.in_use;
>  
> -    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
> +    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
> +                   sizeof(mac_data.entries));
> +    mac_data.entries = ldl_p(&mac_data.entries);
> +    if (s != sizeof(mac_data.entries)) {
> +        return VIRTIO_NET_ERR;
> +    }
> +
> +    iov_discard_front(&iov, &iov_cnt, s);
>  
> -    if (sizeof(mac_data.entries) +
> -        (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
> +    if (mac_data.entries * ETH_ALEN != iov_size(iov, iov_cnt)) {
>          return VIRTIO_NET_ERR;
> +    }
>  
> -    if (mac_data.entries) {
> -        if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
> -            memcpy(n->mac_table.macs + (n->mac_table.in_use * ETH_ALEN),
> -                   elem->out_sg[2].iov_base + sizeof(mac_data),
> -                   mac_data.entries * ETH_ALEN);
> -            n->mac_table.in_use += mac_data.entries;
> -        } else {
> -            n->mac_table.multi_overflow = 1;
> +    if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
> +        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
> +                       mac_data.entries * ETH_ALEN);
> +        if (s != mac_data.entries * ETH_ALEN) {
> +            return VIRTIO_NET_ERR;
>          }
> +        n->mac_table.in_use += mac_data.entries;
> +    } else {
> +        n->mac_table.multi_overflow = 1;
>      }
>  
>      return VIRTIO_NET_OK;
>  }
>  
>  static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
> -                                        VirtQueueElement *elem)
> +                                        struct iovec *iov, unsigned int iov_cnt)
>  {
>      uint16_t vid;
> +    size_t s;
>  
> -    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(vid)) {
> -        error_report("virtio-net ctrl invalid vlan command");
> +    s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
> +    vid = lduw_p(&vid);
> +    if (s != sizeof(vid)) {
>          return VIRTIO_NET_ERR;
>      }
>  
> -    vid = lduw_p(elem->out_sg[1].iov_base);
> -
>      if (vid >= MAX_VLAN)
>          return VIRTIO_NET_ERR;
>  
> @@ -427,30 +445,33 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>      struct virtio_net_ctrl_hdr ctrl;
>      virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>      VirtQueueElement elem;
> +    size_t s;
> +    struct iovec *iov;
> +    unsigned int iov_cnt;
>  
>      while (virtqueue_pop(vq, &elem)) {
> -        if ((elem.in_num < 1) || (elem.out_num < 1)) {
> +        if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
> +            iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
>              error_report("virtio-net ctrl missing headers");
>              exit(1);
>          }
>  
> -        if (elem.out_sg[0].iov_len < sizeof(ctrl) ||
> -            elem.in_sg[elem.in_num - 1].iov_len < sizeof(status)) {
> -            error_report("virtio-net ctrl header not in correct element");
> -            exit(1);
> +        iov = elem.out_sg;
> +        iov_cnt = 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)) {
> +            status = VIRTIO_NET_ERR;
> +        } else if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE) {
> +            status = virtio_net_handle_rx_mode(n, ctrl.cmd, iov, iov_cnt);
> +        } else if (ctrl.class == VIRTIO_NET_CTRL_MAC) {
> +            status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt);
> +        } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) {
> +            status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, iov_cnt);
>          }
>  
> -        ctrl.class = ldub_p(elem.out_sg[0].iov_base);
> -        ctrl.cmd = ldub_p(elem.out_sg[0].iov_base + sizeof(ctrl.class));
> -
> -        if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE)
> -            status = virtio_net_handle_rx_mode(n, ctrl.cmd, &elem);
> -        else if (ctrl.class == VIRTIO_NET_CTRL_MAC)
> -            status = virtio_net_handle_mac(n, ctrl.cmd, &elem);
> -        else if (ctrl.class == VIRTIO_NET_CTRL_VLAN)
> -            status = virtio_net_handle_vlan_table(n, ctrl.cmd, &elem);
> -
> -        stb_p(elem.in_sg[elem.in_num - 1].iov_base, status);
> +        s = iov_from_buf(elem.in_sg, elem.in_num, 0, &status, sizeof(status));
> +        assert(s == sizeof(status));
>  
>          virtqueue_push(vq, &elem, sizeof(status));
>          virtio_notify(vdev, vq);
> -- 
> 1.7.1
diff mbox

Patch

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 3bb01b1..af1f3a1 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -315,44 +315,44 @@  static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
 }
 
 static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
-                                     VirtQueueElement *elem)
+                                     struct iovec *iov, unsigned int iov_cnt)
 {
     uint8_t on;
+    size_t s;
 
-    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(on)) {
-        error_report("virtio-net ctrl invalid rx mode command");
-        exit(1);
+    s = iov_to_buf(iov, iov_cnt, 0, &on, sizeof(on));
+    if (s != sizeof(on)) {
+        return VIRTIO_NET_ERR;
     }
 
-    on = ldub_p(elem->out_sg[1].iov_base);
-
-    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC)
+    if (cmd == VIRTIO_NET_CTRL_RX_MODE_PROMISC) {
         n->promisc = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLMULTI) {
         n->allmulti = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_ALLUNI) {
         n->alluni = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOMULTI) {
         n->nomulti = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOUNI) {
         n->nouni = on;
-    else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST)
+    } else if (cmd == VIRTIO_NET_CTRL_RX_MODE_NOBCAST) {
         n->nobcast = on;
-    else
+    } else {
         return VIRTIO_NET_ERR;
+    }
 
     return VIRTIO_NET_OK;
 }
 
 static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
-                                 VirtQueueElement *elem)
+                                 struct iovec *iov, unsigned int iov_cnt)
 {
     struct virtio_net_ctrl_mac mac_data;
+    size_t s;
 
-    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET || elem->out_num != 3 ||
-        elem->out_sg[1].iov_len < sizeof(mac_data) ||
-        elem->out_sg[2].iov_len < sizeof(mac_data))
+    if (cmd != VIRTIO_NET_CTRL_MAC_TABLE_SET) {
         return VIRTIO_NET_ERR;
+    }
 
     n->mac_table.in_use = 0;
     n->mac_table.first_multi = 0;
@@ -360,54 +360,72 @@  static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
     n->mac_table.multi_overflow = 0;
     memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
 
-    mac_data.entries = ldl_p(elem->out_sg[1].iov_base);
+    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
+                   sizeof(mac_data.entries));
+    mac_data.entries = ldl_p(&mac_data.entries);
+    if (s != sizeof(mac_data.entries)) {
+        return VIRTIO_NET_ERR;
+    }
+    iov_discard_front(&iov, &iov_cnt, s);
 
-    if (sizeof(mac_data.entries) +
-        (mac_data.entries * ETH_ALEN) > elem->out_sg[1].iov_len)
+    if (mac_data.entries * ETH_ALEN > iov_size(iov, iov_cnt)) {
         return VIRTIO_NET_ERR;
+    }
 
     if (mac_data.entries <= MAC_TABLE_ENTRIES) {
-        memcpy(n->mac_table.macs, elem->out_sg[1].iov_base + sizeof(mac_data),
-               mac_data.entries * ETH_ALEN);
+        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
+                       mac_data.entries * ETH_ALEN);
+        if (s != mac_data.entries * ETH_ALEN) {
+            return VIRTIO_NET_ERR;
+        }
         n->mac_table.in_use += mac_data.entries;
     } else {
         n->mac_table.uni_overflow = 1;
     }
 
+    iov_discard_front(&iov, &iov_cnt, mac_data.entries * ETH_ALEN);
+
     n->mac_table.first_multi = n->mac_table.in_use;
 
-    mac_data.entries = ldl_p(elem->out_sg[2].iov_base);
+    s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
+                   sizeof(mac_data.entries));
+    mac_data.entries = ldl_p(&mac_data.entries);
+    if (s != sizeof(mac_data.entries)) {
+        return VIRTIO_NET_ERR;
+    }
+
+    iov_discard_front(&iov, &iov_cnt, s);
 
-    if (sizeof(mac_data.entries) +
-        (mac_data.entries * ETH_ALEN) > elem->out_sg[2].iov_len)
+    if (mac_data.entries * ETH_ALEN != iov_size(iov, iov_cnt)) {
         return VIRTIO_NET_ERR;
+    }
 
-    if (mac_data.entries) {
-        if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
-            memcpy(n->mac_table.macs + (n->mac_table.in_use * ETH_ALEN),
-                   elem->out_sg[2].iov_base + sizeof(mac_data),
-                   mac_data.entries * ETH_ALEN);
-            n->mac_table.in_use += mac_data.entries;
-        } else {
-            n->mac_table.multi_overflow = 1;
+    if (n->mac_table.in_use + mac_data.entries <= MAC_TABLE_ENTRIES) {
+        s = iov_to_buf(iov, iov_cnt, 0, n->mac_table.macs,
+                       mac_data.entries * ETH_ALEN);
+        if (s != mac_data.entries * ETH_ALEN) {
+            return VIRTIO_NET_ERR;
         }
+        n->mac_table.in_use += mac_data.entries;
+    } else {
+        n->mac_table.multi_overflow = 1;
     }
 
     return VIRTIO_NET_OK;
 }
 
 static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
-                                        VirtQueueElement *elem)
+                                        struct iovec *iov, unsigned int iov_cnt)
 {
     uint16_t vid;
+    size_t s;
 
-    if (elem->out_num != 2 || elem->out_sg[1].iov_len != sizeof(vid)) {
-        error_report("virtio-net ctrl invalid vlan command");
+    s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
+    vid = lduw_p(&vid);
+    if (s != sizeof(vid)) {
         return VIRTIO_NET_ERR;
     }
 
-    vid = lduw_p(elem->out_sg[1].iov_base);
-
     if (vid >= MAX_VLAN)
         return VIRTIO_NET_ERR;
 
@@ -427,30 +445,33 @@  static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     struct virtio_net_ctrl_hdr ctrl;
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     VirtQueueElement elem;
+    size_t s;
+    struct iovec *iov;
+    unsigned int iov_cnt;
 
     while (virtqueue_pop(vq, &elem)) {
-        if ((elem.in_num < 1) || (elem.out_num < 1)) {
+        if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
+            iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
             error_report("virtio-net ctrl missing headers");
             exit(1);
         }
 
-        if (elem.out_sg[0].iov_len < sizeof(ctrl) ||
-            elem.in_sg[elem.in_num - 1].iov_len < sizeof(status)) {
-            error_report("virtio-net ctrl header not in correct element");
-            exit(1);
+        iov = elem.out_sg;
+        iov_cnt = 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)) {
+            status = VIRTIO_NET_ERR;
+        } else if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE) {
+            status = virtio_net_handle_rx_mode(n, ctrl.cmd, iov, iov_cnt);
+        } else if (ctrl.class == VIRTIO_NET_CTRL_MAC) {
+            status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt);
+        } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) {
+            status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, iov_cnt);
         }
 
-        ctrl.class = ldub_p(elem.out_sg[0].iov_base);
-        ctrl.cmd = ldub_p(elem.out_sg[0].iov_base + sizeof(ctrl.class));
-
-        if (ctrl.class == VIRTIO_NET_CTRL_RX_MODE)
-            status = virtio_net_handle_rx_mode(n, ctrl.cmd, &elem);
-        else if (ctrl.class == VIRTIO_NET_CTRL_MAC)
-            status = virtio_net_handle_mac(n, ctrl.cmd, &elem);
-        else if (ctrl.class == VIRTIO_NET_CTRL_VLAN)
-            status = virtio_net_handle_vlan_table(n, ctrl.cmd, &elem);
-
-        stb_p(elem.in_sg[elem.in_num - 1].iov_base, status);
+        s = iov_from_buf(elem.in_sg, elem.in_num, 0, &status, sizeof(status));
+        assert(s == sizeof(status));
 
         virtqueue_push(vq, &elem, sizeof(status));
         virtio_notify(vdev, vq);