[Qemu-devel] [PATCH 0/4] QID path collision fix

[Qemu-devel] [PATCH 0/4] QID path collision fix

[antonios.motakis]

From: Antonios Motakis <antonios.motakis@huawei.com>

This is an implementation that ensures that a unique 64bit QID path
is generated per file.

Since if a file is unique on the host is determined by both inode
and device id, which are 64 and 32 bits respectively, the proper
long term fix will be an extension in the 9p protocol allowing for
a bigger QID path field. The fix presented here is a 'fallback'
solution, within the current limitations of 9p.

The first patch is a preparatory change.

The main feature of the second patch is that now stat_to_qid can
fail. We ensure always the same device is being accessed for each
file we advertise, to avoid collisions and leave the interesting
parts for the next patches.

The third patch implements a hash map that maps the first 16 bits of
inode and device id to a 16 bit prefix for the QID path. The last
48 bits are set equal to the last 48 inode bits. This should cover
most users, and should keep the hash table used for the mapping
very small for 99% of use cases.

The fourth patch implements a full mapping, as suggested by Emilio
G. Cota, that kicks in if the prefix map method fails. This can
happen for example if the host OS does not allocate inodes in a
predictable way. In that case, the implementation will allow to
keep track of up to 2**48 QID paths.

Antonios Motakis (4):
  9pfs: V9fsQID: set type of version and path to unsigned
  9pfs: check for file device to avoid QID path collisions
  9pfs: stat_to_qid: use device as input to qid.path
  9pfs: stat_to_qid: implement slow path

 fsdev/9p-marshal.h |   4 +-
 hw/9pfs/9p.c       | 201 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/9pfs/9p.h       |  21 ++++++
 3 files changed, 205 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] 9pfs: V9fsQID: set type of version and path to unsigned

[antonios.motakis]

From: Antonios Motakis <antonios.motakis@huawei.com>

There is no need for signedness on these QID fields for 9p.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
---
 fsdev/9p-marshal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
index c8823d8..d1ad364 100644
--- a/fsdev/9p-marshal.h
+++ b/fsdev/9p-marshal.h
@@ -10,8 +10,8 @@ typedef struct V9fsString
 typedef struct V9fsQID
 {
     int8_t type;
-    int32_t version;
-    int64_t path;
+    uint32_t version;
+    uint64_t path;
 } V9fsQID;
 
 typedef struct V9fsStat
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] 9pfs: check for file device to avoid QID path collisions

[antonios.motakis]

From: Antonios Motakis <antonios.motakis@huawei.com>

The QID path should uniquely identify a file. However, the
inode of a file is currently used as the QID path, which
on its own only uniquely identifies wiles within a device.
Here we track the device hosting the 9pfs share, in order
to prevent security issues with QID path collisions from
other devices.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
---
 hw/9pfs/9p.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 hw/9pfs/9p.h |  1 +
 2 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 85a1ed8..4da858f 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -573,10 +573,16 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
                                 P9_STAT_MODE_SOCKET)
 
 /* This is the algorithm from ufs in spfs */
-static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
+static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
 {
     size_t size;
 
+    if (pdu->s->dev_id == 0) {
+        pdu->s->dev_id = stbuf->st_dev;
+    } else if (pdu->s->dev_id != stbuf->st_dev) {
+        return -ENOSYS;
+    }
+
     memset(&qidp->path, 0, sizeof(qidp->path));
     size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
     memcpy(&qidp->path, &stbuf->st_ino, size);
@@ -588,6 +594,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
     if (S_ISLNK(stbuf->st_mode)) {
         qidp->type |= P9_QID_TYPE_SYMLINK;
     }
+
+    return 0;
 }
 
 static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
@@ -600,7 +608,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
     if (err < 0) {
         return err;
     }
-    stat_to_qid(&stbuf, qidp);
+    err = stat_to_qid(pdu, &stbuf, qidp);
+    if (err < 0) {
+        return err;
+    }
     return 0;
 }
 
@@ -831,7 +842,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
 
     memset(v9stat, 0, sizeof(*v9stat));
 
-    stat_to_qid(stbuf, &v9stat->qid);
+    err = stat_to_qid(pdu, stbuf, &v9stat->qid);
+    if (err < 0) {
+        return err;
+    }
     v9stat->mode = stat_to_v9mode(stbuf);
     v9stat->atime = stbuf->st_atime;
     v9stat->mtime = stbuf->st_mtime;
@@ -892,7 +906,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
 #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields above */
 
 
-static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
+static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
                                 V9fsStatDotl *v9lstat)
 {
     memset(v9lstat, 0, sizeof(*v9lstat));
@@ -914,7 +928,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
     /* Currently we only support BASIC fields in stat */
     v9lstat->st_result_mask = P9_STATS_BASIC;
 
-    stat_to_qid(stbuf, &v9lstat->qid);
+    return stat_to_qid(pdu, stbuf, &v9lstat->qid);
 }
 
 static void print_sg(struct iovec *sg, int cnt)
@@ -1116,7 +1130,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
     uint64_t request_mask;
     V9fsStatDotl v9stat_dotl;
     V9fsPDU *pdu = opaque;
-    V9fsState *s = pdu->s;
 
     retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
     if (retval < 0) {
@@ -1137,7 +1150,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
     if (retval < 0) {
         goto out;
     }
-    stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
+    retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
+    if (retval < 0) {
+        goto out;
+    }
 
     /*  fill st_gen if requested and supported by underlying fs */
     if (request_mask & P9_STATS_GEN) {
@@ -1377,7 +1393,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
             if (err < 0) {
                 goto out;
             }
-            stat_to_qid(&stbuf, &qid);
+            err = stat_to_qid(pdu, &stbuf, &qid);
+            if (err < 0) {
+                goto out;
+            }
             v9fs_path_copy(&dpath, &path);
         }
         memcpy(&qids[name_idx], &qid, sizeof(qid));
@@ -1477,7 +1496,10 @@ static void coroutine_fn v9fs_open(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     if (S_ISDIR(stbuf.st_mode)) {
         err = v9fs_co_opendir(pdu, fidp);
         if (err < 0) {
@@ -1587,7 +1609,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
         fidp->flags |= FID_NON_RECLAIMABLE;
     }
     iounit =  get_iounit(pdu, &fidp->path);
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     if (err < 0) {
         goto out;
@@ -2308,7 +2333,10 @@ static void coroutine_fn v9fs_create(void *opaque)
         }
     }
     iounit = get_iounit(pdu, &fidp->path);
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
     if (err < 0) {
         goto out;
@@ -2365,7 +2393,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err =  pdu_marshal(pdu, offset, "Q", &qid);
     if (err < 0) {
         goto out;
@@ -3033,7 +3064,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Q", &qid);
     if (err < 0) {
         goto out;
@@ -3191,7 +3225,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
     if (err < 0) {
         goto out;
     }
-    stat_to_qid(&stbuf, &qid);
+    err = stat_to_qid(pdu, &stbuf, &qid);
+    if (err < 0) {
+        goto out;
+    }
     err = pdu_marshal(pdu, offset, "Q", &qid);
     if (err < 0) {
         goto out;
@@ -3589,6 +3626,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
         goto out;
     }
 
+    s->dev_id = 0;
+
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 5ced427..afb4ebd 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -252,6 +252,7 @@ struct V9fsState
     Error *migration_blocker;
     V9fsConf fsconf;
     V9fsQID root_qid;
+    dev_t dev_id;
 };
 
 /* 9p2000.L open flags */
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[antonios.motakis]

From: Antonios Motakis <antonios.motakis@huawei.com>

To support multiple devices on the 9p share, and avoid
qid path collisions we take the device id as input
to generate a unique QID path. The lowest 48 bits of
the path will be set equal to the file inode, and the
top bits will be uniquely assigned based on the top
16 bits of the inode and the device id.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
---
 hw/9pfs/9p.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 hw/9pfs/9p.h | 13 ++++++++-
 2 files changed, 90 insertions(+), 11 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4da858f..f434f05 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -25,6 +25,8 @@
 #include "trace.h"
 #include "migration/blocker.h"
 #include "sysemu/qtest.h"
+#include "exec/tb-hash-xx.h"
+#include "qemu/qht.h"
 
 int open_fd_hw;
 int total_open_fd;
@@ -572,20 +574,82 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
                                 P9_STAT_MODE_NAMED_PIPE |   \
                                 P9_STAT_MODE_SOCKET)
 
-/* This is the algorithm from ufs in spfs */
+
+/* creative abuse of tb_hash_func7, which is based on xxhash */
+static uint32_t qpp_hash(QppEntry e)
+{
+    return tb_hash_func7(e.ino_prefix, e.dev, 0, 0, 0);
+}
+
+static bool qpp_lookup_func(const void *obj, const void *userp)
+{
+    const QppEntry *e1 = obj, *e2 = userp;
+    return (e1->dev == e2->dev) && (e1->ino_prefix == e2->ino_prefix);
+}
+
+static void qpp_table_remove(struct qht *ht, void *p, uint32_t h, void *up)
+{
+    g_free(p);
+}
+
+static void qpp_table_destroy(struct qht *ht)
+{
+    qht_iter(ht, qpp_table_remove, NULL);
+    qht_destroy(ht);
+}
+
+/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
+ * to a unique QID path (64 bits). To avoid having to map and keep track
+ * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
+ * the device id to the 16 highest bits of the QID path. The 48 lowest bits
+ * of the QID path equal to the lowest bits of the inode number.
+ *
+ * This takes advantage of the fact that inode number are usually not
+ * random but allocated sequentially, so we have fewer items to keep
+ * track of.
+ */
+static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
+                                uint64_t *path)
+{
+    QppEntry lookup = {
+        .dev = stbuf->st_dev,
+        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
+    }, *val;
+    uint32_t hash = qpp_hash(lookup);
+
+    val = qht_lookup(&pdu->s->qpp_table, qpp_lookup_func, &lookup, hash);
+
+    if (!val) {
+        if (pdu->s->qp_prefix_next == 0) {
+            /* we ran out of prefixes */
+            return -ENFILE;
+        }
+
+        val = g_malloc0(sizeof(QppEntry));
+        if (!val) {
+            return -ENOMEM;
+        }
+        *val = lookup;
+
+        /* new unique inode prefix and device combo */
+        val->qp_prefix = pdu->s->qp_prefix_next++;
+        qht_insert(&pdu->s->qpp_table, val, hash);
+    }
+
+    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
+    return 0;
+}
+
 static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
 {
-    size_t size;
+    int err;
 
-    if (pdu->s->dev_id == 0) {
-        pdu->s->dev_id = stbuf->st_dev;
-    } else if (pdu->s->dev_id != stbuf->st_dev) {
-        return -ENOSYS;
+    /* map inode+device to qid path (fast path) */
+    err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+    if (err) {
+        return err;
     }
 
-    memset(&qidp->path, 0, sizeof(qidp->path));
-    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
-    memcpy(&qidp->path, &stbuf->st_ino, size);
     qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
     qidp->type = 0;
     if (S_ISDIR(stbuf->st_mode)) {
@@ -3626,7 +3690,9 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
         goto out;
     }
 
-    s->dev_id = 0;
+    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
+    qht_init(&s->qpp_table, 1, QHT_MODE_AUTO_RESIZE);
+    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
 
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
@@ -3641,6 +3707,7 @@ out:
         }
         g_free(s->tag);
         g_free(s->ctx.fs_root);
+        qpp_table_destroy(&s->qpp_table);
         v9fs_path_free(&path);
     }
     return rc;
@@ -3653,6 +3720,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
     }
     fsdev_throttle_cleanup(s->ctx.fst);
     g_free(s->tag);
+    qpp_table_destroy(&s->qpp_table);
     g_free(s->ctx.fs_root);
 }
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index afb4ebd..80428f7 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -8,6 +8,7 @@
 #include "fsdev/9p-iov-marshal.h"
 #include "qemu/thread.h"
 #include "qemu/coroutine.h"
+#include "qemu/qht.h"
 
 enum {
     P9_TLERROR = 6,
@@ -231,6 +232,15 @@ struct V9fsFidState
     V9fsFidState *rclm_lst;
 };
 
+#define QPATH_INO_MASK        (((unsigned long)1 << 48) - 1)
+
+/* QID path prefix entry, see stat_to_qid */
+typedef struct {
+    dev_t dev;
+    uint16_t ino_prefix;
+    uint16_t qp_prefix;
+} QppEntry;
+
 struct V9fsState
 {
     QLIST_HEAD(, V9fsPDU) free_list;
@@ -252,7 +262,8 @@ struct V9fsState
     Error *migration_blocker;
     V9fsConf fsconf;
     V9fsQID root_qid;
-    dev_t dev_id;
+    struct qht qpp_table;
+    uint16_t qp_prefix_next;
 };
 
 /* 9p2000.L open flags */
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] 9pfs: stat_to_qid: implement slow path

[antonios.motakis]

From: Antonios Motakis <antonios.motakis@huawei.com>

stat_to_qid attempts via qid_path_prefixmap to map unique files
(which are identified by 64bt inode nr and 32 bit device id)
to a 64 QID path value. However this implementation makes some
assumptions about inode number generation on the host.

If qid_path_prefixmap fails, we still have 48bits available in
the QID path to fall back to a less memory efficient full mapping.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
---
 hw/9pfs/9p.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/9pfs/9p.h |  9 +++++++++
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index f434f05..ae7845d 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -581,23 +581,72 @@ static uint32_t qpp_hash(QppEntry e)
     return tb_hash_func7(e.ino_prefix, e.dev, 0, 0, 0);
 }
 
+static uint32_t qpf_hash(QpfEntry e)
+{
+    return tb_hash_func7(e.ino, e.dev, 0, 0, 0);
+}
+
 static bool qpp_lookup_func(const void *obj, const void *userp)
 {
     const QppEntry *e1 = obj, *e2 = userp;
     return (e1->dev == e2->dev) && (e1->ino_prefix == e2->ino_prefix);
 }
 
-static void qpp_table_remove(struct qht *ht, void *p, uint32_t h, void *up)
+static bool qpf_lookup_func(const void *obj, const void *userp)
+{
+    const QpfEntry *e1 = obj, *e2 = userp;
+    return (e1->dev == e2->dev) && (e1->ino == e2->ino);
+}
+
+static void qp_table_remove(struct qht *ht, void *p, uint32_t h, void *up)
 {
     g_free(p);
 }
 
-static void qpp_table_destroy(struct qht *ht)
+static void qp_table_destroy(struct qht *ht)
 {
-    qht_iter(ht, qpp_table_remove, NULL);
+    qht_iter(ht, qp_table_remove, NULL);
     qht_destroy(ht);
 }
 
+static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
+                            uint64_t *path)
+{
+    QpfEntry lookup = {
+        .dev = stbuf->st_dev,
+        .ino = stbuf->st_ino
+    }, *val;
+    uint32_t hash = qpf_hash(lookup);
+
+    /* most users won't need the fullmap, so init the table lazily */
+    if (!pdu->s->qpf_table.map) {
+        qht_init(&pdu->s->qpf_table, 1 << 16, QHT_MODE_AUTO_RESIZE);
+    }
+
+    val = qht_lookup(&pdu->s->qpf_table, qpf_lookup_func, &lookup, hash);
+
+    if (!val) {
+        if (pdu->s->qp_fullpath_next == 0) {
+            /* no more files can be mapped :'( */
+            return -ENFILE;
+        }
+
+        val = g_malloc0(sizeof(QppEntry));
+        if (!val) {
+            return -ENOMEM;
+        }
+        *val = lookup;
+
+        /* new unique inode and device combo */
+        val->path = pdu->s->qp_fullpath_next++;
+        pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
+        qht_insert(&pdu->s->qpf_table, val, hash);
+    }
+
+    *path = val->path;
+    return 0;
+}
+
 /* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
  * to a unique QID path (64 bits). To avoid having to map and keep track
  * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
@@ -646,6 +695,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
 
     /* map inode+device to qid path (fast path) */
     err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
+    if (err == -ENFILE) {
+        /* fast path didn't work, fal back to full map */
+        err = qid_path_fullmap(pdu, stbuf, &qidp->path);
+    }
     if (err) {
         return err;
     }
@@ -3693,6 +3746,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
     /* QID path hash table. 1 entry ought to be enough for anybody ;) */
     qht_init(&s->qpp_table, 1, QHT_MODE_AUTO_RESIZE);
     s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
+    s->qp_fullpath_next = 1;
 
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
@@ -3707,7 +3761,8 @@ out:
         }
         g_free(s->tag);
         g_free(s->ctx.fs_root);
-        qpp_table_destroy(&s->qpp_table);
+        qp_table_destroy(&s->qpp_table);
+        qp_table_destroy(&s->qpf_table);
         v9fs_path_free(&path);
     }
     return rc;
@@ -3720,7 +3775,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
     }
     fsdev_throttle_cleanup(s->ctx.fst);
     g_free(s->tag);
-    qpp_table_destroy(&s->qpp_table);
+    qp_table_destroy(&s->qpp_table);
+    qp_table_destroy(&s->qpf_table);
     g_free(s->ctx.fs_root);
 }
 
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 80428f7..b561f4a 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -241,6 +241,13 @@ typedef struct {
     uint16_t qp_prefix;
 } QppEntry;
 
+/* QID path full entry, as above */
+typedef struct {
+    dev_t dev;
+    ino_t ino;
+    uint64_t path;
+} QpfEntry;
+
 struct V9fsState
 {
     QLIST_HEAD(, V9fsPDU) free_list;
@@ -263,7 +270,9 @@ struct V9fsState
     V9fsConf fsconf;
     V9fsQID root_qid;
     struct qht qpp_table;
+    struct qht qpf_table;
     uint16_t qp_prefix_next;
+    uint64_t qp_fullpath_next;
 };
 
 /* 9p2000.L open flags */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/4] 9pfs: V9fsQID: set type of version and path to unsigned

[Greg Kurz]

On Thu, 8 Feb 2018 19:00:16 +0100
<antonios.motakis@huawei.com> wrote:

> From: Antonios Motakis <antonios.motakis@huawei.com>
> 
> There is no need for signedness on these QID fields for 9p.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> ---

I agree these should be unsigned, but you have some more places to adapt
to this change. At least these:
- related traces in hw/9pfs/trace-events should then expect unsigned values
- donttouch_stat() in hw/9pfs/9p.c should stop comparing them to -1

>  fsdev/9p-marshal.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
> index c8823d8..d1ad364 100644
> --- a/fsdev/9p-marshal.h
> +++ b/fsdev/9p-marshal.h
> @@ -10,8 +10,8 @@ typedef struct V9fsString
>  typedef struct V9fsQID
>  {
>      int8_t type;

Even if your series doesn't use it, while here, let's drop the sign from
@type as well.

> -    int32_t version;
> -    int64_t path;
> +    uint32_t version;
> +    uint64_t path;
>  } V9fsQID;
>  
>  typedef struct V9fsStat

Re: [Qemu-devel] [PATCH 2/4] 9pfs: check for file device to avoid QID path collisions

[Greg Kurz]

On Thu, 8 Feb 2018 19:00:17 +0100
<antonios.motakis@huawei.com> wrote:

> From: Antonios Motakis <antonios.motakis@huawei.com>
> 
> The QID path should uniquely identify a file. However, the
> inode of a file is currently used as the QID path, which
> on its own only uniquely identifies wiles within a device.
                                      ^^^^^
                                    s/wiles/files

> Here we track the device hosting the 9pfs share, in order
> to prevent security issues with QID path collisions from
> other devices.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> ---
>  hw/9pfs/9p.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++-------------
>  hw/9pfs/9p.h |  1 +
>  2 files changed, 54 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 85a1ed8..4da858f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -573,10 +573,16 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>                                  P9_STAT_MODE_SOCKET)
>  
>  /* This is the algorithm from ufs in spfs */
> -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>  {
>      size_t size;
>  
> +    if (pdu->s->dev_id == 0) {

This check would then be performed by nearly all
requests. I'd rather set stbuf->st_dev at realize
time since we lstat() the root of the 9pfs share.

> +        pdu->s->dev_id = stbuf->st_dev;
> +    } else if (pdu->s->dev_id != stbuf->st_dev) {
> +        return -ENOSYS;

Not sure ENOSYS is an appropriate return value for most guest
syscalls that would land here. Maybe ENOENT like if the path
doesn't exist instead ?

This being said, this st_dev related logic gets reverted in the
next patch IIUC... maybe this patch should just care to add a
return value to stat_to_qid() ?

> +    }
> +
>      memset(&qidp->path, 0, sizeof(qidp->path));
>      size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
>      memcpy(&qidp->path, &stbuf->st_ino, size);
> @@ -588,6 +594,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
>      if (S_ISLNK(stbuf->st_mode)) {
>          qidp->type |= P9_QID_TYPE_SYMLINK;
>      }
> +
> +    return 0;
>  }
>  
>  static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> @@ -600,7 +608,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
>      if (err < 0) {
>          return err;
>      }
> -    stat_to_qid(&stbuf, qidp);
> +    err = stat_to_qid(pdu, &stbuf, qidp);
> +    if (err < 0) {
> +        return err;
> +    }
>      return 0;
>  }
>  
> @@ -831,7 +842,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>  
>      memset(v9stat, 0, sizeof(*v9stat));
>  
> -    stat_to_qid(stbuf, &v9stat->qid);
> +    err = stat_to_qid(pdu, stbuf, &v9stat->qid);
> +    if (err < 0) {
> +        return err;
> +    }
>      v9stat->mode = stat_to_v9mode(stbuf);
>      v9stat->atime = stbuf->st_atime;
>      v9stat->mtime = stbuf->st_mtime;
> @@ -892,7 +906,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>  #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields above */
>  
>  
> -static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
> +static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>                                  V9fsStatDotl *v9lstat)
>  {
>      memset(v9lstat, 0, sizeof(*v9lstat));
> @@ -914,7 +928,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
>      /* Currently we only support BASIC fields in stat */
>      v9lstat->st_result_mask = P9_STATS_BASIC;
>  
> -    stat_to_qid(stbuf, &v9lstat->qid);
> +    return stat_to_qid(pdu, stbuf, &v9lstat->qid);
>  }
>  
>  static void print_sg(struct iovec *sg, int cnt)
> @@ -1116,7 +1130,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>      uint64_t request_mask;
>      V9fsStatDotl v9stat_dotl;
>      V9fsPDU *pdu = opaque;
> -    V9fsState *s = pdu->s;
>  
>      retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
>      if (retval < 0) {
> @@ -1137,7 +1150,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>      if (retval < 0) {
>          goto out;
>      }
> -    stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
> +    retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
> +    if (retval < 0) {
> +        goto out;
> +    }
>  
>      /*  fill st_gen if requested and supported by underlying fs */
>      if (request_mask & P9_STATS_GEN) {
> @@ -1377,7 +1393,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
>              if (err < 0) {
>                  goto out;
>              }
> -            stat_to_qid(&stbuf, &qid);
> +            err = stat_to_qid(pdu, &stbuf, &qid);
> +            if (err < 0) {
> +                goto out;
> +            }
>              v9fs_path_copy(&dpath, &path);
>          }
>          memcpy(&qids[name_idx], &qid, sizeof(qid));
> @@ -1477,7 +1496,10 @@ static void coroutine_fn v9fs_open(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      if (S_ISDIR(stbuf.st_mode)) {
>          err = v9fs_co_opendir(pdu, fidp);
>          if (err < 0) {
> @@ -1587,7 +1609,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
>          fidp->flags |= FID_NON_RECLAIMABLE;
>      }
>      iounit =  get_iounit(pdu, &fidp->path);
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
>      if (err < 0) {
>          goto out;
> @@ -2308,7 +2333,10 @@ static void coroutine_fn v9fs_create(void *opaque)
>          }
>      }
>      iounit = get_iounit(pdu, &fidp->path);
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
>      if (err < 0) {
>          goto out;
> @@ -2365,7 +2393,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err =  pdu_marshal(pdu, offset, "Q", &qid);
>      if (err < 0) {
>          goto out;
> @@ -3033,7 +3064,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Q", &qid);
>      if (err < 0) {
>          goto out;
> @@ -3191,7 +3225,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    stat_to_qid(&stbuf, &qid);
> +    err = stat_to_qid(pdu, &stbuf, &qid);
> +    if (err < 0) {
> +        goto out;
> +    }
>      err = pdu_marshal(pdu, offset, "Q", &qid);
>      if (err < 0) {
>          goto out;
> @@ -3589,6 +3626,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>          goto out;
>      }
>  
> +    s->dev_id = 0;
> +
>      s->ctx.fst = &fse->fst;
>      fsdev_throttle_init(s->ctx.fst);
>  
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 5ced427..afb4ebd 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -252,6 +252,7 @@ struct V9fsState
>      Error *migration_blocker;
>      V9fsConf fsconf;
>      V9fsQID root_qid;
> +    dev_t dev_id;
>  };
>  
>  /* 9p2000.L open flags */

Re: [Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[Greg Kurz]

On Thu, 8 Feb 2018 19:00:18 +0100
<antonios.motakis@huawei.com> wrote:

> From: Antonios Motakis <antonios.motakis@huawei.com>
> 
> To support multiple devices on the 9p share, and avoid
> qid path collisions we take the device id as input
> to generate a unique QID path. The lowest 48 bits of
> the path will be set equal to the file inode, and the
> top bits will be uniquely assigned based on the top
> 16 bits of the inode and the device id.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> ---
>  hw/9pfs/9p.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  hw/9pfs/9p.h | 13 ++++++++-
>  2 files changed, 90 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 4da858f..f434f05 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -25,6 +25,8 @@
>  #include "trace.h"
>  #include "migration/blocker.h"
>  #include "sysemu/qtest.h"
> +#include "exec/tb-hash-xx.h"
> +#include "qemu/qht.h"
>  
>  int open_fd_hw;
>  int total_open_fd;
> @@ -572,20 +574,82 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>                                  P9_STAT_MODE_NAMED_PIPE |   \
>                                  P9_STAT_MODE_SOCKET)
>  
> -/* This is the algorithm from ufs in spfs */
> +
> +/* creative abuse of tb_hash_func7, which is based on xxhash */
> +static uint32_t qpp_hash(QppEntry e)
> +{
> +    return tb_hash_func7(e.ino_prefix, e.dev, 0, 0, 0);

Hmm... Looking at git log include/exec/tb-hash-xx.h, I see that this hash
function signature evolved according to TCG needs. It started with 3
arguments, then 4 and we have 5 today.

So I don't think we should add another unrelated user. Probably best to
have our own version. Also it seems it could be simpler since you always
pass 0 for the third and later arguments.

> +}
> +
> +static bool qpp_lookup_func(const void *obj, const void *userp)
> +{
> +    const QppEntry *e1 = obj, *e2 = userp;
> +    return (e1->dev == e2->dev) && (e1->ino_prefix == e2->ino_prefix);

I guess this expression is simple enough so that you can drop the
parenthesis, since they're not needed because of '==' having
precedence over '&&'.

See: http://en.cppreference.com/w/c/language/operator_precedence

> +}
> +
> +static void qpp_table_remove(struct qht *ht, void *p, uint32_t h, void *up)
> +{
> +    g_free(p);
> +}
> +
> +static void qpp_table_destroy(struct qht *ht)
> +{
> +    qht_iter(ht, qpp_table_remove, NULL);
> +    qht_destroy(ht);
> +}
> +
> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
> + * to a unique QID path (64 bits). To avoid having to map and keep track
> + * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
> + * the device id to the 16 highest bits of the QID path. The 48 lowest bits
> + * of the QID path equal to the lowest bits of the inode number.
> + *
> + * This takes advantage of the fact that inode number are usually not
> + * random but allocated sequentially, so we have fewer items to keep
> + * track of.
> + */
> +static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
> +                                uint64_t *path)
> +{
> +    QppEntry lookup = {
> +        .dev = stbuf->st_dev,
> +        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
> +    }, *val;
> +    uint32_t hash = qpp_hash(lookup);
> +
> +    val = qht_lookup(&pdu->s->qpp_table, qpp_lookup_func, &lookup, hash);
> +
> +    if (!val) {
> +        if (pdu->s->qp_prefix_next == 0) {
> +            /* we ran out of prefixes */
> +            return -ENFILE;

Not sure this errno would make sense for guest syscalls that don't open
file descriptors... Maybe ENOENT ?

Cc'ing Eric for insights.

> +        }
> +
> +        val = g_malloc0(sizeof(QppEntry));
> +        if (!val) {
> +            return -ENOMEM;
> +        }
> +        *val = lookup;
> +
> +        /* new unique inode prefix and device combo */
> +        val->qp_prefix = pdu->s->qp_prefix_next++;
> +        qht_insert(&pdu->s->qpp_table, val, hash);
> +    }
> +
> +    *path = ((uint64_t)val->qp_prefix << 48) | (stbuf->st_ino & QPATH_INO_MASK);
> +    return 0;
> +}
> +
>  static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>  {
> -    size_t size;
> +    int err;
>  
> -    if (pdu->s->dev_id == 0) {
> -        pdu->s->dev_id = stbuf->st_dev;
> -    } else if (pdu->s->dev_id != stbuf->st_dev) {
> -        return -ENOSYS;
> +    /* map inode+device to qid path (fast path) */
> +    err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> +    if (err) {
> +        return err;
>      }
>  
> -    memset(&qidp->path, 0, sizeof(qidp->path));
> -    size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> -    memcpy(&qidp->path, &stbuf->st_ino, size);
>      qidp->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
>      qidp->type = 0;
>      if (S_ISDIR(stbuf->st_mode)) {
> @@ -3626,7 +3690,9 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>          goto out;
>      }
>  
> -    s->dev_id = 0;
> +    /* QID path hash table. 1 entry ought to be enough for anybody ;) */
> +    qht_init(&s->qpp_table, 1, QHT_MODE_AUTO_RESIZE);
> +    s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
>  
>      s->ctx.fst = &fse->fst;
>      fsdev_throttle_init(s->ctx.fst);
> @@ -3641,6 +3707,7 @@ out:
>          }
>          g_free(s->tag);
>          g_free(s->ctx.fs_root);
> +        qpp_table_destroy(&s->qpp_table);
>          v9fs_path_free(&path);
>      }
>      return rc;
> @@ -3653,6 +3720,7 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
>      }
>      fsdev_throttle_cleanup(s->ctx.fst);
>      g_free(s->tag);
> +    qpp_table_destroy(&s->qpp_table);
>      g_free(s->ctx.fs_root);
>  }
>  
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index afb4ebd..80428f7 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -8,6 +8,7 @@
>  #include "fsdev/9p-iov-marshal.h"
>  #include "qemu/thread.h"
>  #include "qemu/coroutine.h"
> +#include "qemu/qht.h"
>  
>  enum {
>      P9_TLERROR = 6,
> @@ -231,6 +232,15 @@ struct V9fsFidState
>      V9fsFidState *rclm_lst;
>  };
>  
> +#define QPATH_INO_MASK        (((unsigned long)1 << 48) - 1)
> +
> +/* QID path prefix entry, see stat_to_qid */
> +typedef struct {
> +    dev_t dev;
> +    uint16_t ino_prefix;
> +    uint16_t qp_prefix;
> +} QppEntry;
> +
>  struct V9fsState
>  {
>      QLIST_HEAD(, V9fsPDU) free_list;
> @@ -252,7 +262,8 @@ struct V9fsState
>      Error *migration_blocker;
>      V9fsConf fsconf;
>      V9fsQID root_qid;
> -    dev_t dev_id;
> +    struct qht qpp_table;
> +    uint16_t qp_prefix_next;
>  };
>  
>  /* 9p2000.L open flags */

Re: [Qemu-devel] [PATCH 4/4] 9pfs: stat_to_qid: implement slow path

[Greg Kurz]

On Thu, 8 Feb 2018 19:00:19 +0100
<antonios.motakis@huawei.com> wrote:

> From: Antonios Motakis <antonios.motakis@huawei.com>
> 
> stat_to_qid attempts via qid_path_prefixmap to map unique files
> (which are identified by 64bt inode nr and 32 bit device id)
> to a 64 QID path value. However this implementation makes some
> assumptions about inode number generation on the host.
> 
> If qid_path_prefixmap fails, we still have 48bits available in
> the QID path to fall back to a less memory efficient full mapping.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> ---
>  hw/9pfs/9p.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  hw/9pfs/9p.h |  9 +++++++++
>  2 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index f434f05..ae7845d 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -581,23 +581,72 @@ static uint32_t qpp_hash(QppEntry e)
>      return tb_hash_func7(e.ino_prefix, e.dev, 0, 0, 0);
>  }
>  
> +static uint32_t qpf_hash(QpfEntry e)
> +{
> +    return tb_hash_func7(e.ino, e.dev, 0, 0, 0);

Same remark as with previous patch, obviously :)

> +}
> +
>  static bool qpp_lookup_func(const void *obj, const void *userp)
>  {
>      const QppEntry *e1 = obj, *e2 = userp;
>      return (e1->dev == e2->dev) && (e1->ino_prefix == e2->ino_prefix);

Parenthesitis ? ;)

>  }
>  
> -static void qpp_table_remove(struct qht *ht, void *p, uint32_t h, void *up)
> +static bool qpf_lookup_func(const void *obj, const void *userp)
> +{
> +    const QpfEntry *e1 = obj, *e2 = userp;
> +    return (e1->dev == e2->dev) && (e1->ino == e2->ino);

Parenthesitis ? ;)

> +}
> +
> +static void qp_table_remove(struct qht *ht, void *p, uint32_t h, void *up)
>  {
>      g_free(p);
>  }
>  
> -static void qpp_table_destroy(struct qht *ht)
> +static void qp_table_destroy(struct qht *ht)
>  {
> -    qht_iter(ht, qpp_table_remove, NULL);
> +    qht_iter(ht, qp_table_remove, NULL);
>      qht_destroy(ht);
>  }
>  
> +static int qid_path_fullmap(V9fsPDU *pdu, const struct stat *stbuf,
> +                            uint64_t *path)
> +{
> +    QpfEntry lookup = {
> +        .dev = stbuf->st_dev,
> +        .ino = stbuf->st_ino
> +    }, *val;
> +    uint32_t hash = qpf_hash(lookup);
> +
> +    /* most users won't need the fullmap, so init the table lazily */
> +    if (!pdu->s->qpf_table.map) {
> +        qht_init(&pdu->s->qpf_table, 1 << 16, QHT_MODE_AUTO_RESIZE);
> +    }
> +
> +    val = qht_lookup(&pdu->s->qpf_table, qpf_lookup_func, &lookup, hash);
> +
> +    if (!val) {
> +        if (pdu->s->qp_fullpath_next == 0) {
> +            /* no more files can be mapped :'( */
> +            return -ENFILE;
> +        }
> +
> +        val = g_malloc0(sizeof(QppEntry));
> +        if (!val) {
> +            return -ENOMEM;
> +        }
> +        *val = lookup;
> +
> +        /* new unique inode and device combo */
> +        val->path = pdu->s->qp_fullpath_next++;
> +        pdu->s->qp_fullpath_next &= QPATH_INO_MASK;
> +        qht_insert(&pdu->s->qpf_table, val, hash);
> +    }
> +
> +    *path = val->path;
> +    return 0;
> +}
> +
>  /* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
>   * to a unique QID path (64 bits). To avoid having to map and keep track
>   * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
> @@ -646,6 +695,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>  
>      /* map inode+device to qid path (fast path) */
>      err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> +    if (err == -ENFILE) {
> +        /* fast path didn't work, fal back to full map */
> +        err = qid_path_fullmap(pdu, stbuf, &qidp->path);

Hmm... if we have already generate QIDs with fast path, how
can we be sure we won't collide with the ones from the full
map ?

IIRC, Emilio had suggested to use bit 63 to distinguish between
fast and slow path.

> +    }
>      if (err) {
>          return err;
>      }
> @@ -3693,6 +3746,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>      /* QID path hash table. 1 entry ought to be enough for anybody ;) */
>      qht_init(&s->qpp_table, 1, QHT_MODE_AUTO_RESIZE);
>      s->qp_prefix_next = 1; /* reserve 0 to detect overflow */
> +    s->qp_fullpath_next = 1;
>  
>      s->ctx.fst = &fse->fst;
>      fsdev_throttle_init(s->ctx.fst);
> @@ -3707,7 +3761,8 @@ out:
>          }
>          g_free(s->tag);
>          g_free(s->ctx.fs_root);
> -        qpp_table_destroy(&s->qpp_table);
> +        qp_table_destroy(&s->qpp_table);
> +        qp_table_destroy(&s->qpf_table);
>          v9fs_path_free(&path);
>      }
>      return rc;
> @@ -3720,7 +3775,8 @@ void v9fs_device_unrealize_common(V9fsState *s, Error **errp)
>      }
>      fsdev_throttle_cleanup(s->ctx.fst);
>      g_free(s->tag);
> -    qpp_table_destroy(&s->qpp_table);
> +    qp_table_destroy(&s->qpp_table);
> +    qp_table_destroy(&s->qpf_table);
>      g_free(s->ctx.fs_root);
>  }
>  
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 80428f7..b561f4a 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -241,6 +241,13 @@ typedef struct {
>      uint16_t qp_prefix;
>  } QppEntry;
>  
> +/* QID path full entry, as above */
> +typedef struct {
> +    dev_t dev;
> +    ino_t ino;
> +    uint64_t path;
> +} QpfEntry;
> +
>  struct V9fsState
>  {
>      QLIST_HEAD(, V9fsPDU) free_list;
> @@ -263,7 +270,9 @@ struct V9fsState
>      V9fsConf fsconf;
>      V9fsQID root_qid;
>      struct qht qpp_table;
> +    struct qht qpf_table;
>      uint16_t qp_prefix_next;
> +    uint64_t qp_fullpath_next;
>  };
>  
>  /* 9p2000.L open flags */

Re: [Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[Eric Blake]

On 02/09/2018 09:13 AM, Greg Kurz wrote:
> On Thu, 8 Feb 2018 19:00:18 +0100
> <antonios.motakis@huawei.com> wrote:
> 
>> From: Antonios Motakis <antonios.motakis@huawei.com>
>>
>> To support multiple devices on the 9p share, and avoid
>> qid path collisions we take the device id as input
>> to generate a unique QID path. The lowest 48 bits of
>> the path will be set equal to the file inode, and the
>> top bits will be uniquely assigned based on the top
>> 16 bits of the inode and the device id.

>> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
>> + * to a unique QID path (64 bits). To avoid having to map and keep track
>> + * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
>> + * the device id to the 16 highest bits of the QID path. The 48 lowest bits
>> + * of the QID path equal to the lowest bits of the inode number.
>> + *
>> + * This takes advantage of the fact that inode number are usually not
>> + * random but allocated sequentially, so we have fewer items to keep
>> + * track of.
>> + */
>> +static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
>> +                                uint64_t *path)
>> +{
>> +    QppEntry lookup = {
>> +        .dev = stbuf->st_dev,
>> +        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
>> +    }, *val;
>> +    uint32_t hash = qpp_hash(lookup);
>> +
>> +    val = qht_lookup(&pdu->s->qpp_table, qpp_lookup_func, &lookup, hash);
>> +
>> +    if (!val) {
>> +        if (pdu->s->qp_prefix_next == 0) {
>> +            /* we ran out of prefixes */
>> +            return -ENFILE;
> 
> Not sure this errno would make sense for guest syscalls that don't open
> file descriptors... Maybe ENOENT ?
> 
> Cc'ing Eric for insights.

Actually, it makes sense to me:

ENFILE 23 Too many open files in system

You only get here if the hash table filled up, which means there are 
indeed too many open files (even if this syscall wasn't opening a file). 
  In fact, ENFILE is usually associated with running into ulimit 
restrictions, and come to think of it, you are more likely to hit your 
ulimit than you are to run into a hash collision (so this code may be 
very hard to reach in practice, but if you do reach it, it behaves 
similarly to what you were more likely to hit in the first place).

ENOENT implies the file doesn't exist - but here, the file exists but we 
can't open it because we're out of resources for tracking it.

Also, POSIX permits returning specific errno codes that aren't otherwise 
listed for a syscall if the usual semantics of that errno code are 
indeed the reason for the failure (you should prefer to fail with errno 
codes documented by POSIX where possible, but POSIX does not limit you 
to just that set).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[Greg Kurz]

On Fri, 9 Feb 2018 10:06:05 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 02/09/2018 09:13 AM, Greg Kurz wrote:
> > On Thu, 8 Feb 2018 19:00:18 +0100
> > <antonios.motakis@huawei.com> wrote:
> >   
> >> From: Antonios Motakis <antonios.motakis@huawei.com>
> >>
> >> To support multiple devices on the 9p share, and avoid
> >> qid path collisions we take the device id as input
> >> to generate a unique QID path. The lowest 48 bits of
> >> the path will be set equal to the file inode, and the
> >> top bits will be uniquely assigned based on the top
> >> 16 bits of the inode and the device id.  
> 
> >> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
> >> + * to a unique QID path (64 bits). To avoid having to map and keep track
> >> + * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
> >> + * the device id to the 16 highest bits of the QID path. The 48 lowest bits
> >> + * of the QID path equal to the lowest bits of the inode number.
> >> + *
> >> + * This takes advantage of the fact that inode number are usually not
> >> + * random but allocated sequentially, so we have fewer items to keep
> >> + * track of.
> >> + */
> >> +static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
> >> +                                uint64_t *path)
> >> +{
> >> +    QppEntry lookup = {
> >> +        .dev = stbuf->st_dev,
> >> +        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
> >> +    }, *val;
> >> +    uint32_t hash = qpp_hash(lookup);
> >> +
> >> +    val = qht_lookup(&pdu->s->qpp_table, qpp_lookup_func, &lookup, hash);
> >> +
> >> +    if (!val) {
> >> +        if (pdu->s->qp_prefix_next == 0) {
> >> +            /* we ran out of prefixes */
> >> +            return -ENFILE;  
> > 
> > Not sure this errno would make sense for guest syscalls that don't open
> > file descriptors... Maybe ENOENT ?
> > 
> > Cc'ing Eric for insights.  
> 
> Actually, it makes sense to me:
> 
> ENFILE 23 Too many open files in system
> 
> You only get here if the hash table filled up, which means there are 
> indeed too many open files (even if this syscall wasn't opening a file). 
>   In fact, ENFILE is usually associated with running into ulimit 
> restrictions, and come to think of it, you are more likely to hit your 
> ulimit than you are to run into a hash collision (so this code may be 
> very hard to reach in practice, but if you do reach it, it behaves 
> similarly to what you were more likely to hit in the first place).
> 
> ENOENT implies the file doesn't exist - but here, the file exists but we 
> can't open it because we're out of resources for tracking it.
> 

Only the host knows that the file exists actually. If stat_to_qid() for
this path returns ENOENT, then the file shouldn't be visible in the
guest.

I say "shouldn't" instead of "isn't" because I now realize that
v9fs_do_readdir(), which is used to implement getdents() in the
guest, sends a degraded QID, hand-crafted without calling
stat_to_qid() at all... :-\

        /*
         * Fill up just the path field of qid because the client uses
         * only that. To fill the entire qid structure we will have
         * to stat each dirent found, which is expensive
         */
        size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
        memcpy(&qid.path, &dent->d_ino, size);
        /* Fill the other fields with dummy values */
        qid.type = 0;
        qid.version = 0;


Antonios, your patchset should probably also address this.

The problem is that the dirent we get from v9fs_co_readdir()
only has the inode number, so I guess we must build a dummy
struct stat with:

    stbuf.st_ino = dent->d_ino
    stbuf.st_dev = st_dev of the parent directory

The st_dev of the parent directory could be obtained in
v9fs_readdir() and passed to v9fs_do_readdir(). Another
solution could be to cache the QID in the V9fsFidState
of the parent directory when it is open.

Also, if we hit a collision while reading the directory, I'm
afraid the remaining entries won't be read at all. I'm not
sure this is really what we want.

> Also, POSIX permits returning specific errno codes that aren't otherwise 
> listed for a syscall if the usual semantics of that errno code are 
> indeed the reason for the failure (you should prefer to fail with errno 
> codes documented by POSIX where possible, but POSIX does not limit you 
> to just that set).
> 

Ok, then ENFILE wouldn't be that bad in the end. 

Thanks for your POSIX expertise :)

Cheers,

--
Greg

Re: [Qemu-devel] [PATCH 4/4] 9pfs: stat_to_qid: implement slow path

[Emilio G. Cota]

On Fri, Feb 09, 2018 at 16:22:33 +0100, Greg Kurz wrote:
> On Thu, 8 Feb 2018 19:00:19 +0100
> <antonios.motakis@huawei.com> wrote:
(snip)
> >  /* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
> >   * to a unique QID path (64 bits). To avoid having to map and keep track
> >   * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
> > @@ -646,6 +695,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
> >  
> >      /* map inode+device to qid path (fast path) */
> >      err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
> > +    if (err == -ENFILE) {
> > +        /* fast path didn't work, fal back to full map */
> > +        err = qid_path_fullmap(pdu, stbuf, &qidp->path);
> 
> Hmm... if we have already generate QIDs with fast path, how
> can we be sure we won't collide with the ones from the full
> map ?
> 
> IIRC, Emilio had suggested to use bit 63 to distinguish between
> fast and slow path.

Yep. Antonios: did you consider that approach? For reference:
  https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05084.html

That would make the fast path faster by just using bit masks, which I
think it's a superior approach if indeed the assumption about top bits
being zero in most cases is accurate.

		Emilio

Re: [Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[Emilio G. Cota]

On Fri, Feb 09, 2018 at 16:13:26 +0100, Greg Kurz wrote:
> On Thu, 8 Feb 2018 19:00:18 +0100
> <antonios.motakis@huawei.com> wrote:
(snip)
> > +/* creative abuse of tb_hash_func7, which is based on xxhash */
> > +static uint32_t qpp_hash(QppEntry e)
> > +{
> > +    return tb_hash_func7(e.ino_prefix, e.dev, 0, 0, 0);
> 
> Hmm... Looking at git log include/exec/tb-hash-xx.h, I see that this hash
> function signature evolved according to TCG needs. It started with 3
> arguments, then 4 and we have 5 today.
> 
> So I don't think we should add another unrelated user. Probably best to
> have our own version. Also it seems it could be simpler since you always
> pass 0 for the third and later arguments.

It's fine to use it; the compiler will do the right thing
with those '0' arguments, because the function is always inlined.

That said, I have some cleanup patches to export this as xxhash and make
tb_hash a user of it. The patches also remove most of the ugly 0's from
callers. Those patches will have to wait for a bit though, so for now
just use tb_hash_func7.

(snip)
> > +        val = g_malloc0(sizeof(QppEntry));
> > +        if (!val) {
> > +            return -ENOMEM;
> > +        }

g_malloc0 and all other glib memory allocation functions only
return NULL if the size argument is 0.
If out of memory, the application is terminated, which is why
this check is not needed.
Reference:
  https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#glib-Memory-Allocation.description

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH 1/4] 9pfs: V9fsQID: set type of version and path to unsigned

[Antonios Motakis]

On 02/09/2018 01:37 PM, Greg Kurz wrote:
> On Thu, 8 Feb 2018 19:00:16 +0100
> <antonios.motakis@huawei.com> wrote:
>
>> From: Antonios Motakis <antonios.motakis@huawei.com>
>>
>> There is no need for signedness on these QID fields for 9p.
>>
>> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
>> ---
> I agree these should be unsigned, but you have some more places to adapt
> to this change. At least these:
> - related traces in hw/9pfs/trace-events should then expect unsigned values
> - donttouch_stat() in hw/9pfs/9p.c should stop comparing them to -1

Noted!

>
>>   fsdev/9p-marshal.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
>> index c8823d8..d1ad364 100644
>> --- a/fsdev/9p-marshal.h
>> +++ b/fsdev/9p-marshal.h
>> @@ -10,8 +10,8 @@ typedef struct V9fsString
>>   typedef struct V9fsQID
>>   {
>>       int8_t type;
> Even if your series doesn't use it, while here, let's drop the sign from
> @type as well.
Agreed

>
>> -    int32_t version;
>> -    int64_t path;
>> +    uint32_t version;
>> +    uint64_t path;
>>   } V9fsQID;
>>   
>>   typedef struct V9fsStat

Re: [Qemu-devel] [PATCH 2/4] 9pfs: check for file device to avoid QID path collisions

[Antonios Motakis]

On 02/09/2018 02:03 PM, Greg Kurz wrote:
> On Thu, 8 Feb 2018 19:00:17 +0100
> <antonios.motakis@huawei.com> wrote:
>
>> From: Antonios Motakis <antonios.motakis@huawei.com>
>>
>> The QID path should uniquely identify a file. However, the
>> inode of a file is currently used as the QID path, which
>> on its own only uniquely identifies wiles within a device.
>                                        ^^^^^
>                                      s/wiles/files
>
>> Here we track the device hosting the 9pfs share, in order
>> to prevent security issues with QID path collisions from
>> other devices.
>>
>> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
>> ---
>>   hw/9pfs/9p.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++-------------
>>   hw/9pfs/9p.h |  1 +
>>   2 files changed, 54 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 85a1ed8..4da858f 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -573,10 +573,16 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>>                                   P9_STAT_MODE_SOCKET)
>>   
>>   /* This is the algorithm from ufs in spfs */
>> -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
>> +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>>   {
>>       size_t size;
>>   
>> +    if (pdu->s->dev_id == 0) {
> This check would then be performed by nearly all
> requests. I'd rather set stbuf->st_dev at realize
> time since we lstat() the root of the 9pfs share.
>
>> +        pdu->s->dev_id = stbuf->st_dev;
>> +    } else if (pdu->s->dev_id != stbuf->st_dev) {
>> +        return -ENOSYS;
> Not sure ENOSYS is an appropriate return value for most guest
> syscalls that would land here. Maybe ENOENT like if the path
> doesn't exist instead ?
>
> This being said, this st_dev related logic gets reverted in the
> next patch IIUC... maybe this patch should just care to add a
> return value to stat_to_qid() ?

Indeed, I just wanted to keep a separate patch that handles that 
stat_to_qid should be able to return failure. We can remove the st_dev 
checks.

>
>> +    }
>> +
>>       memset(&qidp->path, 0, sizeof(qidp->path));
>>       size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
>>       memcpy(&qidp->path, &stbuf->st_ino, size);
>> @@ -588,6 +594,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
>>       if (S_ISLNK(stbuf->st_mode)) {
>>           qidp->type |= P9_QID_TYPE_SYMLINK;
>>       }
>> +
>> +    return 0;
>>   }
>>   
>>   static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
>> @@ -600,7 +608,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
>>       if (err < 0) {
>>           return err;
>>       }
>> -    stat_to_qid(&stbuf, qidp);
>> +    err = stat_to_qid(pdu, &stbuf, qidp);
>> +    if (err < 0) {
>> +        return err;
>> +    }
>>       return 0;
>>   }
>>   
>> @@ -831,7 +842,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>>   
>>       memset(v9stat, 0, sizeof(*v9stat));
>>   
>> -    stat_to_qid(stbuf, &v9stat->qid);
>> +    err = stat_to_qid(pdu, stbuf, &v9stat->qid);
>> +    if (err < 0) {
>> +        return err;
>> +    }
>>       v9stat->mode = stat_to_v9mode(stbuf);
>>       v9stat->atime = stbuf->st_atime;
>>       v9stat->mtime = stbuf->st_mtime;
>> @@ -892,7 +906,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *path,
>>   #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields above */
>>   
>>   
>> -static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
>> +static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
>>                                   V9fsStatDotl *v9lstat)
>>   {
>>       memset(v9lstat, 0, sizeof(*v9lstat));
>> @@ -914,7 +928,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
>>       /* Currently we only support BASIC fields in stat */
>>       v9lstat->st_result_mask = P9_STATS_BASIC;
>>   
>> -    stat_to_qid(stbuf, &v9lstat->qid);
>> +    return stat_to_qid(pdu, stbuf, &v9lstat->qid);
>>   }
>>   
>>   static void print_sg(struct iovec *sg, int cnt)
>> @@ -1116,7 +1130,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>>       uint64_t request_mask;
>>       V9fsStatDotl v9stat_dotl;
>>       V9fsPDU *pdu = opaque;
>> -    V9fsState *s = pdu->s;
>>   
>>       retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
>>       if (retval < 0) {
>> @@ -1137,7 +1150,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
>>       if (retval < 0) {
>>           goto out;
>>       }
>> -    stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
>> +    retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
>> +    if (retval < 0) {
>> +        goto out;
>> +    }
>>   
>>       /*  fill st_gen if requested and supported by underlying fs */
>>       if (request_mask & P9_STATS_GEN) {
>> @@ -1377,7 +1393,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
>>               if (err < 0) {
>>                   goto out;
>>               }
>> -            stat_to_qid(&stbuf, &qid);
>> +            err = stat_to_qid(pdu, &stbuf, &qid);
>> +            if (err < 0) {
>> +                goto out;
>> +            }
>>               v9fs_path_copy(&dpath, &path);
>>           }
>>           memcpy(&qids[name_idx], &qid, sizeof(qid));
>> @@ -1477,7 +1496,10 @@ static void coroutine_fn v9fs_open(void *opaque)
>>       if (err < 0) {
>>           goto out;
>>       }
>> -    stat_to_qid(&stbuf, &qid);
>> +    err = stat_to_qid(pdu, &stbuf, &qid);
>> +    if (err < 0) {
>> +        goto out;
>> +    }
>>       if (S_ISDIR(stbuf.st_mode)) {
>>           err = v9fs_co_opendir(pdu, fidp);
>>           if (err < 0) {
>> @@ -1587,7 +1609,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
>>           fidp->flags |= FID_NON_RECLAIMABLE;
>>       }
>>       iounit =  get_iounit(pdu, &fidp->path);
>> -    stat_to_qid(&stbuf, &qid);
>> +    err = stat_to_qid(pdu, &stbuf, &qid);
>> +    if (err < 0) {
>> +        goto out;
>> +    }
>>       err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
>>       if (err < 0) {
>>           goto out;
>> @@ -2308,7 +2333,10 @@ static void coroutine_fn v9fs_create(void *opaque)
>>           }
>>       }
>>       iounit = get_iounit(pdu, &fidp->path);
>> -    stat_to_qid(&stbuf, &qid);
>> +    err = stat_to_qid(pdu, &stbuf, &qid);
>> +    if (err < 0) {
>> +        goto out;
>> +    }
>>       err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
>>       if (err < 0) {
>>           goto out;
>> @@ -2365,7 +2393,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
>>       if (err < 0) {
>>           goto out;
>>       }
>> -    stat_to_qid(&stbuf, &qid);
>> +    err = stat_to_qid(pdu, &stbuf, &qid);
>> +    if (err < 0) {
>> +        goto out;
>> +    }
>>       err =  pdu_marshal(pdu, offset, "Q", &qid);
>>       if (err < 0) {
>>           goto out;
>> @@ -3033,7 +3064,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
>>       if (err < 0) {
>>           goto out;
>>       }
>> -    stat_to_qid(&stbuf, &qid);
>> +    err = stat_to_qid(pdu, &stbuf, &qid);
>> +    if (err < 0) {
>> +        goto out;
>> +    }
>>       err = pdu_marshal(pdu, offset, "Q", &qid);
>>       if (err < 0) {
>>           goto out;
>> @@ -3191,7 +3225,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
>>       if (err < 0) {
>>           goto out;
>>       }
>> -    stat_to_qid(&stbuf, &qid);
>> +    err = stat_to_qid(pdu, &stbuf, &qid);
>> +    if (err < 0) {
>> +        goto out;
>> +    }
>>       err = pdu_marshal(pdu, offset, "Q", &qid);
>>       if (err < 0) {
>>           goto out;
>> @@ -3589,6 +3626,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>>           goto out;
>>       }
>>   
>> +    s->dev_id = 0;
>> +
>>       s->ctx.fst = &fse->fst;
>>       fsdev_throttle_init(s->ctx.fst);
>>   
>> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
>> index 5ced427..afb4ebd 100644
>> --- a/hw/9pfs/9p.h
>> +++ b/hw/9pfs/9p.h
>> @@ -252,6 +252,7 @@ struct V9fsState
>>       Error *migration_blocker;
>>       V9fsConf fsconf;
>>       V9fsQID root_qid;
>> +    dev_t dev_id;
>>   };
>>   
>>   /* 9p2000.L open flags */

Re: [Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[Antonios Motakis]

On 02/09/2018 10:58 PM, Emilio G. Cota wrote:
> On Fri, Feb 09, 2018 at 16:13:26 +0100, Greg Kurz wrote:
>> On Thu, 8 Feb 2018 19:00:18 +0100
>> <antonios.motakis@huawei.com> wrote:
> (snip)
>>> +/* creative abuse of tb_hash_func7, which is based on xxhash */
>>> +static uint32_t qpp_hash(QppEntry e)
>>> +{
>>> +    return tb_hash_func7(e.ino_prefix, e.dev, 0, 0, 0);
>> Hmm... Looking at git log include/exec/tb-hash-xx.h, I see that this hash
>> function signature evolved according to TCG needs. It started with 3
>> arguments, then 4 and we have 5 today.
>>
>> So I don't think we should add another unrelated user. Probably best to
>> have our own version. Also it seems it could be simpler since you always
>> pass 0 for the third and later arguments.
> It's fine to use it; the compiler will do the right thing
> with those '0' arguments, because the function is always inlined.
>
> That said, I have some cleanup patches to export this as xxhash and make
> tb_hash a user of it. The patches also remove most of the ugly 0's from
> callers. Those patches will have to wait for a bit though, so for now
> just use tb_hash_func7.

I guess this is why it's a good idea to share patches even if still rough ;)
So I will not worry about this part for now.

>
> (snip)
>>> +        val = g_malloc0(sizeof(QppEntry));
>>> +        if (!val) {
>>> +            return -ENOMEM;
>>> +        }
> g_malloc0 and all other glib memory allocation functions only
> return NULL if the size argument is 0.
> If out of memory, the application is terminated, which is why
> this check is not needed.
> Reference:
>    https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#glib-Memory-Allocation.description

Thanks for the clarification.

>
> Thanks,
>
> 		Emilio

Re: [Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[Antonios Motakis]

On 02/09/2018 06:57 PM, Greg Kurz wrote:
> On Fri, 9 Feb 2018 10:06:05 -0600
> Eric Blake <eblake@redhat.com> wrote:
>
>> On 02/09/2018 09:13 AM, Greg Kurz wrote:
>>> On Thu, 8 Feb 2018 19:00:18 +0100
>>> <antonios.motakis@huawei.com> wrote:
>>>    
>>>> From: Antonios Motakis <antonios.motakis@huawei.com>
>>>>
>>>> To support multiple devices on the 9p share, and avoid
>>>> qid path collisions we take the device id as input
>>>> to generate a unique QID path. The lowest 48 bits of
>>>> the path will be set equal to the file inode, and the
>>>> top bits will be uniquely assigned based on the top
>>>> 16 bits of the inode and the device id.
>>>> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
>>>> + * to a unique QID path (64 bits). To avoid having to map and keep track
>>>> + * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
>>>> + * the device id to the 16 highest bits of the QID path. The 48 lowest bits
>>>> + * of the QID path equal to the lowest bits of the inode number.
>>>> + *
>>>> + * This takes advantage of the fact that inode number are usually not
>>>> + * random but allocated sequentially, so we have fewer items to keep
>>>> + * track of.
>>>> + */
>>>> +static int qid_path_prefixmap(V9fsPDU *pdu, const struct stat *stbuf,
>>>> +                                uint64_t *path)
>>>> +{
>>>> +    QppEntry lookup = {
>>>> +        .dev = stbuf->st_dev,
>>>> +        .ino_prefix = (uint16_t) (stbuf->st_ino >> 48)
>>>> +    }, *val;
>>>> +    uint32_t hash = qpp_hash(lookup);
>>>> +
>>>> +    val = qht_lookup(&pdu->s->qpp_table, qpp_lookup_func, &lookup, hash);
>>>> +
>>>> +    if (!val) {
>>>> +        if (pdu->s->qp_prefix_next == 0) {
>>>> +            /* we ran out of prefixes */
>>>> +            return -ENFILE;
>>> Not sure this errno would make sense for guest syscalls that don't open
>>> file descriptors... Maybe ENOENT ?
>>>
>>> Cc'ing Eric for insights.
>> Actually, it makes sense to me:
>>
>> ENFILE 23 Too many open files in system
>>
>> You only get here if the hash table filled up, which means there are
>> indeed too many open files (even if this syscall wasn't opening a file).
>>    In fact, ENFILE is usually associated with running into ulimit
>> restrictions, and come to think of it, you are more likely to hit your
>> ulimit than you are to run into a hash collision (so this code may be
>> very hard to reach in practice, but if you do reach it, it behaves
>> similarly to what you were more likely to hit in the first place).
>>
>> ENOENT implies the file doesn't exist - but here, the file exists but we
>> can't open it because we're out of resources for tracking it.
>>
> Only the host knows that the file exists actually. If stat_to_qid() for
> this path returns ENOENT, then the file shouldn't be visible in the
> guest.
>
> I say "shouldn't" instead of "isn't" because I now realize that
> v9fs_do_readdir(), which is used to implement getdents() in the
> guest, sends a degraded QID, hand-crafted without calling
> stat_to_qid() at all... :-\
>
>          /*
>           * Fill up just the path field of qid because the client uses
>           * only that. To fill the entire qid structure we will have
>           * to stat each dirent found, which is expensive
>           */
>          size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
>          memcpy(&qid.path, &dent->d_ino, size);
>          /* Fill the other fields with dummy values */
>          qid.type = 0;
>          qid.version = 0;
>
>
> Antonios, your patchset should probably also address this.
>
> The problem is that the dirent we get from v9fs_co_readdir()
> only has the inode number, so I guess we must build a dummy
> struct stat with:
>
>      stbuf.st_ino = dent->d_ino
>      stbuf.st_dev = st_dev of the parent directory
>
> The st_dev of the parent directory could be obtained in
> v9fs_readdir() and passed to v9fs_do_readdir(). Another
> solution could be to cache the QID in the V9fsFidState
> of the parent directory when it is open.
>
> Also, if we hit a collision while reading the directory, I'm
> afraid the remaining entries won't be read at all. I'm not
> sure this is really what we want.
Good catch!

I'm not that sure that we can make the assumption that all entries in a 
dir will share the st_dev,
I think we have to check it for each entry...

>
>> Also, POSIX permits returning specific errno codes that aren't otherwise
>> listed for a syscall if the usual semantics of that errno code are
>> indeed the reason for the failure (you should prefer to fail with errno
>> codes documented by POSIX where possible, but POSIX does not limit you
>> to just that set).
>>
> Ok, then ENFILE wouldn't be that bad in the end.
>
> Thanks for your POSIX expertise :)
Will keep that one then!

>
> Cheers,
>
> --
> Greg

Re: [Qemu-devel] [PATCH 4/4] 9pfs: stat_to_qid: implement slow path

[Antonios Motakis]

On 02/09/2018 10:47 PM, Emilio G. Cota wrote:
> On Fri, Feb 09, 2018 at 16:22:33 +0100, Greg Kurz wrote:
>> On Thu, 8 Feb 2018 19:00:19 +0100
>> <antonios.motakis@huawei.com> wrote:
> (snip)
>>>   /* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
>>>    * to a unique QID path (64 bits). To avoid having to map and keep track
>>>    * of up to 2^64 objects, we map only the 16 highest bits of the inode plus
>>> @@ -646,6 +695,10 @@ static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
>>>   
>>>       /* map inode+device to qid path (fast path) */
>>>       err = qid_path_prefixmap(pdu, stbuf, &qidp->path);
>>> +    if (err == -ENFILE) {
>>> +        /* fast path didn't work, fal back to full map */
>>> +        err = qid_path_fullmap(pdu, stbuf, &qidp->path);
>> Hmm... if we have already generate QIDs with fast path, how
>> can we be sure we won't collide with the ones from the full
>> map ?
>>
>> IIRC, Emilio had suggested to use bit 63 to distinguish between
>> fast and slow path.
> Yep. Antonios: did you consider that approach? For reference:
>    https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05084.html
>
> That would make the fast path faster by just using bit masks, which I
> think it's a superior approach if indeed the assumption about top bits
> being zero in most cases is accurate.
>
> 		Emilio

The fast path reserves prefix 0x0000 to detect overflows, and so will 
allocate prefixes 0x0001 to 0xffff only.
So if the fast path fails, we still have the whole space of 64 bit 
values that start with 0x0000, and 48 bits of play
room. And this is the space the slow path is allocating from. So they 
will never allocate a colliding path,
prefix 0x0000 distinguishes the slow path.

By reserving one prefix instead of one bit, we keep the bit-space that 
we can work with larger.
We can track almost twice as many QID paths.

I did consider the approach proposed originally using bitmasks, however 
I think this implementation has the advantage:
(1) The fast path is just being checked first without any other pre-checks,
which means the common use case is assumed to be true / is optimized.
We slow down the slow path because we have to check the fast path first,
but personally I don't mind slowing down the slow path a bit.
(2) It is a bit more flexible with the assumptions about the top bits. 
Think about
nested virtualization with nested 9p for example; the inner QEMU 
instance won't have
the luxury of working with inodes with zero top bits. However, the 
combination of
prefixes that it will run into will still be discreet and non-random. 
The proposed approach
will still allow the fast path to work fully for all files.

I think it is plausible that there are other cases with non-zero, but 
also non-randomly distributed
top bits in the inode, so I opted to give the fast path another 
advantage at the expense of the slow path.

I can change it, but personally I have accepted the slow path as a much 
more inferior fallback,
that only 0,001% of users should ever see. Fast path FTW.

Thanks for your feedback!
Tony

Re: [Qemu-devel] [PATCH 3/4] 9pfs: stat_to_qid: use device as input to qid.path

[Greg Kurz]

On Fri, 16 Feb 2018 11:20:33 +0100
Antonios Motakis <antonios.motakis@huawei.com> wrote:

<snip>
> 
> I'm not that sure that we can make the assumption that all entries in a 
> dir will share the st_dev,

The assumption stands for any entry that is not a directory actually.
But indeed a directory could be a mount point, and have a different
st_dev.

> I think we have to check it for each entry...
> 

Only if the entry might be a directory, ie,

dent->d_type == DT_DIR || dent->d_type == DT_UNKNOWN

> >  
> >> Also, POSIX permits returning specific errno codes that aren't otherwise
> >> listed for a syscall if the usual semantics of that errno code are
> >> indeed the reason for the failure (you should prefer to fail with errno
> >> codes documented by POSIX where possible, but POSIX does not limit you
> >> to just that set).
> >>  
> > Ok, then ENFILE wouldn't be that bad in the end.
> >
> > Thanks for your POSIX expertise :)  
> Will keep that one then!
> 
> >
> > Cheers,
> >
> > --
> > Greg  
>