Blame 0008-hw-9pfs-replace-iovec-manipulation-with-QEMUIOVector.patch

Justin M. Forbes 45e84a
From 45d6cdff48356dc8974497ec0524f971b646dd70 Mon Sep 17 00:00:00 2001
Justin M. Forbes 45e84a
From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Justin M. Forbes 45e84a
Date: Wed, 21 Dec 2011 12:37:22 +0530
Justin M. Forbes 45e84a
Subject: [PATCH 08/25] hw/9pfs: replace iovec manipulation with QEMUIOVector
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
The v9fs_read() and v9fs_write() functions rely on iovec[] manipulation
Justin M. Forbes 45e84a
code should be replaced with QEMUIOVector to avoid duplicating code.
Justin M. Forbes 45e84a
In the future it may be possible to make the code even more concise by
Justin M. Forbes 45e84a
using QEMUIOVector consistently across virtio and 9pfs.
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
The "v" format specifier for pdu_marshal() and pdu_unmarshal() is
Justin M. Forbes 45e84a
dropped since it does not actually pack/unpack anything.  The specifier
Justin M. Forbes 45e84a
was also not implemented to update the offset variable and could only be
Justin M. Forbes 45e84a
used at the end of a format string, another sign that this shouldn't
Justin M. Forbes 45e84a
really be a format specifier.  Instead, see the new
Justin M. Forbes 45e84a
v9fs_init_qiov_from_pdu() function.
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
This change avoids a possible iovec[] buffer overflow when indirect
Justin M. Forbes 45e84a
vrings are used since the number of vectors is now limited by the
Justin M. Forbes 45e84a
underlying VirtQueueElement and cannot be out-of-bounds.
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Justin M. Forbes 45e84a
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Justin M. Forbes 45e84a
---
Justin M. Forbes 45e84a
 hw/9pfs/virtio-9p.c |  162 +++++++++++++++++++--------------------------------
Justin M. Forbes 45e84a
 1 files changed, 60 insertions(+), 102 deletions(-)
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
Justin M. Forbes 45e84a
index dd43209..c018916 100644
Justin M. Forbes 45e84a
--- a/hw/9pfs/virtio-9p.c
Justin M. Forbes 45e84a
+++ b/hw/9pfs/virtio-9p.c
Justin M. Forbes 45e84a
@@ -674,40 +674,6 @@ static size_t pdu_pack(V9fsPDU *pdu, size_t offset, const void *src,
Justin M. Forbes 45e84a
                              offset, size, 1);
Justin M. Forbes 45e84a
 }
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
-static int pdu_copy_sg(V9fsPDU *pdu, size_t offset, int rx, struct iovec *sg)
Justin M. Forbes 45e84a
-{
Justin M. Forbes 45e84a
-    size_t pos = 0;
Justin M. Forbes 45e84a
-    int i, j;
Justin M. Forbes 45e84a
-    struct iovec *src_sg;
Justin M. Forbes 45e84a
-    unsigned int num;
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-    if (rx) {
Justin M. Forbes 45e84a
-        src_sg = pdu->elem.in_sg;
Justin M. Forbes 45e84a
-        num = pdu->elem.in_num;
Justin M. Forbes 45e84a
-    } else {
Justin M. Forbes 45e84a
-        src_sg = pdu->elem.out_sg;
Justin M. Forbes 45e84a
-        num = pdu->elem.out_num;
Justin M. Forbes 45e84a
-    }
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-    j = 0;
Justin M. Forbes 45e84a
-    for (i = 0; i < num; i++) {
Justin M. Forbes 45e84a
-        if (offset <= pos) {
Justin M. Forbes 45e84a
-            sg[j].iov_base = src_sg[i].iov_base;
Justin M. Forbes 45e84a
-            sg[j].iov_len = src_sg[i].iov_len;
Justin M. Forbes 45e84a
-            j++;
Justin M. Forbes 45e84a
-        } else if (offset < (src_sg[i].iov_len + pos)) {
Justin M. Forbes 45e84a
-            sg[j].iov_base = src_sg[i].iov_base;
Justin M. Forbes 45e84a
-            sg[j].iov_len = src_sg[i].iov_len;
Justin M. Forbes 45e84a
-            sg[j].iov_base += (offset - pos);
Justin M. Forbes 45e84a
-            sg[j].iov_len -= (offset - pos);
Justin M. Forbes 45e84a
-            j++;
Justin M. Forbes 45e84a
-        }
Justin M. Forbes 45e84a
-        pos += src_sg[i].iov_len;
Justin M. Forbes 45e84a
-    }
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-    return j;
Justin M. Forbes 45e84a
-}
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
 static size_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
Justin M. Forbes 45e84a
 {
Justin M. Forbes 45e84a
     size_t old_offset = offset;
Justin M. Forbes 45e84a
@@ -743,12 +709,6 @@ static size_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
Justin M. Forbes 45e84a
             *valp = le64_to_cpu(val);
Justin M. Forbes 45e84a
             break;
Justin M. Forbes 45e84a
         }
Justin M. Forbes 45e84a
-        case 'v': {
Justin M. Forbes 45e84a
-            struct iovec *iov = va_arg(ap, struct iovec *);
Justin M. Forbes 45e84a
-            int *iovcnt = va_arg(ap, int *);
Justin M. Forbes 45e84a
-            *iovcnt = pdu_copy_sg(pdu, offset, 0, iov);
Justin M. Forbes 45e84a
-            break;
Justin M. Forbes 45e84a
-        }
Justin M. Forbes 45e84a
         case 's': {
Justin M. Forbes 45e84a
             V9fsString *str = va_arg(ap, V9fsString *);
Justin M. Forbes 45e84a
             offset += pdu_unmarshal(pdu, offset, "w", &str->size);
Justin M. Forbes 45e84a
@@ -827,12 +787,6 @@ static size_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
Justin M. Forbes 45e84a
             offset += pdu_pack(pdu, offset, &val, sizeof(val));
Justin M. Forbes 45e84a
             break;
Justin M. Forbes 45e84a
         }
Justin M. Forbes 45e84a
-        case 'v': {
Justin M. Forbes 45e84a
-            struct iovec *iov = va_arg(ap, struct iovec *);
Justin M. Forbes 45e84a
-            int *iovcnt = va_arg(ap, int *);
Justin M. Forbes 45e84a
-            *iovcnt = pdu_copy_sg(pdu, offset, 1, iov);
Justin M. Forbes 45e84a
-            break;
Justin M. Forbes 45e84a
-        }
Justin M. Forbes 45e84a
         case 's': {
Justin M. Forbes 45e84a
             V9fsString *str = va_arg(ap, V9fsString *);
Justin M. Forbes 45e84a
             offset += pdu_marshal(pdu, offset, "w", str->size);
Justin M. Forbes 45e84a
@@ -1143,42 +1097,6 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
Justin M. Forbes 45e84a
     stat_to_qid(stbuf, &v9lstat->qid);
Justin M. Forbes 45e84a
 }
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
-static struct iovec *adjust_sg(struct iovec *sg, int len, int *iovcnt)
Justin M. Forbes 45e84a
-{
Justin M. Forbes 45e84a
-    while (len && *iovcnt) {
Justin M. Forbes 45e84a
-        if (len < sg->iov_len) {
Justin M. Forbes 45e84a
-            sg->iov_len -= len;
Justin M. Forbes 45e84a
-            sg->iov_base += len;
Justin M. Forbes 45e84a
-            len = 0;
Justin M. Forbes 45e84a
-        } else {
Justin M. Forbes 45e84a
-            len -= sg->iov_len;
Justin M. Forbes 45e84a
-            sg++;
Justin M. Forbes 45e84a
-            *iovcnt -= 1;
Justin M. Forbes 45e84a
-        }
Justin M. Forbes 45e84a
-    }
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-    return sg;
Justin M. Forbes 45e84a
-}
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-static struct iovec *cap_sg(struct iovec *sg, int cap, int *cnt)
Justin M. Forbes 45e84a
-{
Justin M. Forbes 45e84a
-    int i;
Justin M. Forbes 45e84a
-    int total = 0;
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-    for (i = 0; i < *cnt; i++) {
Justin M. Forbes 45e84a
-        if ((total + sg[i].iov_len) > cap) {
Justin M. Forbes 45e84a
-            sg[i].iov_len -= ((total + sg[i].iov_len) - cap);
Justin M. Forbes 45e84a
-            i++;
Justin M. Forbes 45e84a
-            break;
Justin M. Forbes 45e84a
-        }
Justin M. Forbes 45e84a
-        total += sg[i].iov_len;
Justin M. Forbes 45e84a
-    }
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-    *cnt = i;
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
-    return sg;
Justin M. Forbes 45e84a
-}
Justin M. Forbes 45e84a
-
Justin M. Forbes 45e84a
 static void print_sg(struct iovec *sg, int cnt)
Justin M. Forbes 45e84a
 {
Justin M. Forbes 45e84a
     int i;
Justin M. Forbes 45e84a
@@ -1861,6 +1779,38 @@ out:
Justin M. Forbes 45e84a
     return count;
Justin M. Forbes 45e84a
 }
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
+/*
Justin M. Forbes 45e84a
+ * Create a QEMUIOVector for a sub-region of PDU iovecs
Justin M. Forbes 45e84a
+ *
Justin M. Forbes 45e84a
+ * @qiov:       uninitialized QEMUIOVector
Justin M. Forbes 45e84a
+ * @skip:       number of bytes to skip from beginning of PDU
Justin M. Forbes 45e84a
+ * @size:       number of bytes to include
Justin M. Forbes 45e84a
+ * @is_write:   true - write, false - read
Justin M. Forbes 45e84a
+ *
Justin M. Forbes 45e84a
+ * The resulting QEMUIOVector has heap-allocated iovecs and must be cleaned up
Justin M. Forbes 45e84a
+ * with qemu_iovec_destroy().
Justin M. Forbes 45e84a
+ */
Justin M. Forbes 45e84a
+static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
Justin M. Forbes 45e84a
+                                    uint64_t skip, size_t size,
Justin M. Forbes 45e84a
+                                    bool is_write)
Justin M. Forbes 45e84a
+{
Justin M. Forbes 45e84a
+    QEMUIOVector elem;
Justin M. Forbes 45e84a
+    struct iovec *iov;
Justin M. Forbes 45e84a
+    unsigned int niov;
Justin M. Forbes 45e84a
+
Justin M. Forbes 45e84a
+    if (is_write) {
Justin M. Forbes 45e84a
+        iov = pdu->elem.out_sg;
Justin M. Forbes 45e84a
+        niov = pdu->elem.out_num;
Justin M. Forbes 45e84a
+    } else {
Justin M. Forbes 45e84a
+        iov = pdu->elem.in_sg;
Justin M. Forbes 45e84a
+        niov = pdu->elem.in_num;
Justin M. Forbes 45e84a
+    }
Justin M. Forbes 45e84a
+
Justin M. Forbes 45e84a
+    qemu_iovec_init_external(&elem, iov, niov);
Justin M. Forbes 45e84a
+    qemu_iovec_init(qiov, niov);
Justin M. Forbes 45e84a
+    qemu_iovec_copy(qiov, &elem, skip, size);
Justin M. Forbes 45e84a
+}
Justin M. Forbes 45e84a
+
Justin M. Forbes 45e84a
 static void v9fs_read(void *opaque)
Justin M. Forbes 45e84a
 {
Justin M. Forbes 45e84a
     int32_t fid;
Justin M. Forbes 45e84a
@@ -1895,21 +1845,21 @@ static void v9fs_read(void *opaque)
Justin M. Forbes 45e84a
         err += pdu_marshal(pdu, offset, "d", count);
Justin M. Forbes 45e84a
         err += count;
Justin M. Forbes 45e84a
     } else if (fidp->fid_type == P9_FID_FILE) {
Justin M. Forbes 45e84a
-        int32_t cnt;
Justin M. Forbes 45e84a
+        QEMUIOVector qiov_full;
Justin M. Forbes 45e84a
+        QEMUIOVector qiov;
Justin M. Forbes 45e84a
         int32_t len;
Justin M. Forbes 45e84a
-        struct iovec *sg;
Justin M. Forbes 45e84a
-        struct iovec iov[128]; /* FIXME: bad, bad, bad */
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
-        sg = iov;
Justin M. Forbes 45e84a
-        pdu_marshal(pdu, offset + 4, "v", sg, &cnt);
Justin M. Forbes 45e84a
-        sg = cap_sg(sg, max_count, &cnt);
Justin M. Forbes 45e84a
+        v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, max_count, false);
Justin M. Forbes 45e84a
+        qemu_iovec_init(&qiov, qiov_full.niov);
Justin M. Forbes 45e84a
         do {
Justin M. Forbes 45e84a
+            qemu_iovec_reset(&qiov);
Justin M. Forbes 45e84a
+            qemu_iovec_copy(&qiov, &qiov_full, count, qiov_full.size - count);
Justin M. Forbes 45e84a
             if (0) {
Justin M. Forbes 45e84a
-                print_sg(sg, cnt);
Justin M. Forbes 45e84a
+                print_sg(qiov.iov, qiov.niov);
Justin M. Forbes 45e84a
             }
Justin M. Forbes 45e84a
             /* Loop in case of EINTR */
Justin M. Forbes 45e84a
             do {
Justin M. Forbes 45e84a
-                len = v9fs_co_preadv(pdu, fidp, sg, cnt, off);
Justin M. Forbes 45e84a
+                len = v9fs_co_preadv(pdu, fidp, qiov.iov, qiov.niov, off);
Justin M. Forbes 45e84a
                 if (len >= 0) {
Justin M. Forbes 45e84a
                     off   += len;
Justin M. Forbes 45e84a
                     count += len;
Justin M. Forbes 45e84a
@@ -1920,11 +1870,12 @@ static void v9fs_read(void *opaque)
Justin M. Forbes 45e84a
                 err = len;
Justin M. Forbes 45e84a
                 goto out;
Justin M. Forbes 45e84a
             }
Justin M. Forbes 45e84a
-            sg = adjust_sg(sg, len, &cnt);
Justin M. Forbes 45e84a
         } while (count < max_count && len > 0);
Justin M. Forbes 45e84a
         err = offset;
Justin M. Forbes 45e84a
         err += pdu_marshal(pdu, offset, "d", count);
Justin M. Forbes 45e84a
         err += count;
Justin M. Forbes 45e84a
+        qemu_iovec_destroy(&qiov);
Justin M. Forbes 45e84a
+        qemu_iovec_destroy(&qiov_full);
Justin M. Forbes 45e84a
     } else if (fidp->fid_type == P9_FID_XATTR) {
Justin M. Forbes 45e84a
         err = v9fs_xattr_read(s, pdu, fidp, off, max_count);
Justin M. Forbes 45e84a
     } else {
Justin M. Forbes 45e84a
@@ -2095,7 +2046,6 @@ out:
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
 static void v9fs_write(void *opaque)
Justin M. Forbes 45e84a
 {
Justin M. Forbes 45e84a
-    int cnt;
Justin M. Forbes 45e84a
     ssize_t err;
Justin M. Forbes 45e84a
     int32_t fid;
Justin M. Forbes 45e84a
     int64_t off;
Justin M. Forbes 45e84a
@@ -2104,13 +2054,14 @@ static void v9fs_write(void *opaque)
Justin M. Forbes 45e84a
     int32_t total = 0;
Justin M. Forbes 45e84a
     size_t offset = 7;
Justin M. Forbes 45e84a
     V9fsFidState *fidp;
Justin M. Forbes 45e84a
-    struct iovec iov[128]; /* FIXME: bad, bad, bad */
Justin M. Forbes 45e84a
-    struct iovec *sg = iov;
Justin M. Forbes 45e84a
     V9fsPDU *pdu = opaque;
Justin M. Forbes 45e84a
     V9fsState *s = pdu->s;
Justin M. Forbes 45e84a
+    QEMUIOVector qiov_full;
Justin M. Forbes 45e84a
+    QEMUIOVector qiov;
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
-    pdu_unmarshal(pdu, offset, "dqdv", &fid, &off, &count, sg, &cnt);
Justin M. Forbes 45e84a
-    trace_v9fs_write(pdu->tag, pdu->id, fid, off, count, cnt);
Justin M. Forbes 45e84a
+    offset += pdu_unmarshal(pdu, offset, "dqd", &fid, &off, &count);
Justin M. Forbes 45e84a
+    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, count, true);
Justin M. Forbes 45e84a
+    trace_v9fs_write(pdu->tag, pdu->id, fid, off, count, qiov_full.niov);
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
     fidp = get_fid(pdu, fid);
Justin M. Forbes 45e84a
     if (fidp == NULL) {
Justin M. Forbes 45e84a
@@ -2126,20 +2077,23 @@ static void v9fs_write(void *opaque)
Justin M. Forbes 45e84a
         /*
Justin M. Forbes 45e84a
          * setxattr operation
Justin M. Forbes 45e84a
          */
Justin M. Forbes 45e84a
-        err = v9fs_xattr_write(s, pdu, fidp, off, count, sg, cnt);
Justin M. Forbes 45e84a
+        err = v9fs_xattr_write(s, pdu, fidp, off, count,
Justin M. Forbes 45e84a
+                               qiov_full.iov, qiov_full.niov);
Justin M. Forbes 45e84a
         goto out;
Justin M. Forbes 45e84a
     } else {
Justin M. Forbes 45e84a
         err = -EINVAL;
Justin M. Forbes 45e84a
         goto out;
Justin M. Forbes 45e84a
     }
Justin M. Forbes 45e84a
-    sg = cap_sg(sg, count, &cnt);
Justin M. Forbes 45e84a
+    qemu_iovec_init(&qiov, qiov_full.niov);
Justin M. Forbes 45e84a
     do {
Justin M. Forbes 45e84a
+        qemu_iovec_reset(&qiov);
Justin M. Forbes 45e84a
+        qemu_iovec_copy(&qiov, &qiov_full, total, qiov_full.size - total);
Justin M. Forbes 45e84a
         if (0) {
Justin M. Forbes 45e84a
-            print_sg(sg, cnt);
Justin M. Forbes 45e84a
+            print_sg(qiov.iov, qiov.niov);
Justin M. Forbes 45e84a
         }
Justin M. Forbes 45e84a
         /* Loop in case of EINTR */
Justin M. Forbes 45e84a
         do {
Justin M. Forbes 45e84a
-            len = v9fs_co_pwritev(pdu, fidp, sg, cnt, off);
Justin M. Forbes 45e84a
+            len = v9fs_co_pwritev(pdu, fidp, qiov.iov, qiov.niov, off);
Justin M. Forbes 45e84a
             if (len >= 0) {
Justin M. Forbes 45e84a
                 off   += len;
Justin M. Forbes 45e84a
                 total += len;
Justin M. Forbes 45e84a
@@ -2148,16 +2102,20 @@ static void v9fs_write(void *opaque)
Justin M. Forbes 45e84a
         if (len < 0) {
Justin M. Forbes 45e84a
             /* IO error return the error */
Justin M. Forbes 45e84a
             err = len;
Justin M. Forbes 45e84a
-            goto out;
Justin M. Forbes 45e84a
+            goto out_qiov;
Justin M. Forbes 45e84a
         }
Justin M. Forbes 45e84a
-        sg = adjust_sg(sg, len, &cnt);
Justin M. Forbes 45e84a
     } while (total < count && len > 0);
Justin M. Forbes 45e84a
+
Justin M. Forbes 45e84a
+    offset = 7;
Justin M. Forbes 45e84a
     offset += pdu_marshal(pdu, offset, "d", total);
Justin M. Forbes 45e84a
     err = offset;
Justin M. Forbes 45e84a
     trace_v9fs_write_return(pdu->tag, pdu->id, total, err);
Justin M. Forbes 45e84a
+out_qiov:
Justin M. Forbes 45e84a
+    qemu_iovec_destroy(&qiov);
Justin M. Forbes 45e84a
 out:
Justin M. Forbes 45e84a
     put_fid(pdu, fidp);
Justin M. Forbes 45e84a
 out_nofid:
Justin M. Forbes 45e84a
+    qemu_iovec_destroy(&qiov_full);
Justin M. Forbes 45e84a
     complete_pdu(s, pdu, err);
Justin M. Forbes 45e84a
 }
Justin M. Forbes 45e84a
Justin M. Forbes 45e84a
-- 
Justin M. Forbes 45e84a
1.7.7.5
Justin M. Forbes 45e84a