[Qemu-devel] [PULL 0/7] 9p patches 2018-06-07

[Qemu-devel] [PULL 0/7] 9p patches 2018-06-07

[Greg Kurz]

The following changes since commit 5d328d7d2f1fd4fb160bcfb6e4eb838720274438:

  Merge remote-tracking branch 'remotes/awilliam/tags/vfio-update-20180605.0' into staging (2018-06-07 08:59:28 +0100)

are available in the Git repository at:

  https://github.com/gkurz/qemu.git tags/for-upstream

for you to fetch changes up to aca6897fba149a2a650dcdf5a5e1ae828371f4aa:

  9p: xattr: Properly translate xattrcreate flags (2018-06-07 12:17:22 +0200)

----------------------------------------------------------------
Mostly bug fixes and code sanitization motivated by the upcoming
support for Darwin hosts. Thanks to Keno Fischer.

----------------------------------------------------------------
Keno Fischer (7):
      9p: proxy: Fix size passed to `connect`
      9p: local: Properly set errp in fstatfs error path
      9p: Move a couple xattr functions to 9p-util
      9p: xattr: Fix crashes due to free of uninitialized value
      9p: local: Avoid warning if FS_IOC_GETVERSION is not defined
      9p: Properly check/translate flags in unlinkat
      9p: xattr: Properly translate xattrcreate flags

 hw/9pfs/9p-handle.c |  8 +-------
 hw/9pfs/9p-local.c  | 40 ++++++++++++++++++++++++----------------
 hw/9pfs/9p-proxy.c  |  5 ++---
 hw/9pfs/9p-util.c   | 33 +++++++++++++++++++++++++++++++++
 hw/9pfs/9p-util.h   |  4 ++++
 hw/9pfs/9p-xattr.c  | 33 ---------------------------------
 hw/9pfs/9p.c        | 34 ++++++++++++++++++++++++++++------
 hw/9pfs/9p.h        |  4 ++++
 8 files changed, 96 insertions(+), 65 deletions(-)
-- 
2.14.4

[Qemu-devel] [PULL 1/7] 9p: proxy: Fix size passed to `connect`

[Greg Kurz]

From: Keno Fischer <keno@juliacomputing.com>

The size to pass to the `connect` call is the size of the entire
`struct sockaddr_un`. Passing anything shorter than this causes errors
on darwin.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-proxy.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index e2e03292de2c..47a94e088daa 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -1088,7 +1088,7 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path,
 
 static int connect_namedsocket(const char *path, Error **errp)
 {
-    int sockfd, size;
+    int sockfd;
     struct sockaddr_un helper;
 
     if (strlen(path) >= sizeof(helper.sun_path)) {
@@ -1102,8 +1102,7 @@ static int connect_namedsocket(const char *path, Error **errp)
     }
     strcpy(helper.sun_path, path);
     helper.sun_family = AF_UNIX;
-    size = strlen(helper.sun_path) + sizeof(helper.sun_family);
-    if (connect(sockfd, (struct sockaddr *)&helper, size) < 0) {
+    if (connect(sockfd, (struct sockaddr *)&helper, sizeof(helper)) < 0) {
         error_setg_errno(errp, errno, "failed to connect to '%s'", path);
         close(sockfd);
         return -1;
-- 
2.14.4

[Qemu-devel] [PULL 2/7] 9p: local: Properly set errp in fstatfs error path

[Greg Kurz]

From: Keno Fischer <keno@juliacomputing.com>

In the review of

    9p: Avoid warning if FS_IOC_GETVERSION is not defined

Grep Kurz noted this error path was failing to set errp.
Fix that.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
[added local: to commit title, Greg Kurz]
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b37b1db453da..7758c3850900 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1418,6 +1418,8 @@ static int local_init(FsContext *ctx, Error **errp)
      */
     if (fstatfs(data->mountfd, &stbuf) < 0) {
         close_preserve_errno(data->mountfd);
+        error_setg_errno(errp, errno,
+            "failed to stat file system at '%s'", ctx->fs_root);
         goto err;
     }
     switch (stbuf.f_type) {
-- 
2.14.4

[Qemu-devel] [PULL 3/7] 9p: Move a couple xattr functions to 9p-util

[Greg Kurz]

From: Keno Fischer <keno@juliacomputing.com>

These functions will need custom implementations on Darwin. Since the
implementation is very similar among all of them, and 9p-util already
has the _nofollow version of fgetxattrat, let's move them all there.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-util.c  | 33 +++++++++++++++++++++++++++++++++
 hw/9pfs/9p-util.h  |  4 ++++
 hw/9pfs/9p-xattr.c | 33 ---------------------------------
 3 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c
index f709c27a1fbd..614b7fc34d54 100644
--- a/hw/9pfs/9p-util.c
+++ b/hw/9pfs/9p-util.c
@@ -24,3 +24,36 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *filename, const char *name,
     g_free(proc_path);
     return ret;
 }
+
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+                              char *list, size_t size)
+{
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+    int ret;
+
+    ret = llistxattr(proc_path, list, size);
+    g_free(proc_path);
+    return ret;
+}
+
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+                                const char *name)
+{
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+    int ret;
+
+    ret = lremovexattr(proc_path, name);
+    g_free(proc_path);
+    return ret;
+}
+
+int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
+                         void *value, size_t size, int flags)
+{
+    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
+    int ret;
+
+    ret = lsetxattr(proc_path, name, value, size, flags);
+    g_free(proc_path);
+    return ret;
+}
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index dc0d2e29aa3b..79ed6b233e58 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -60,5 +60,9 @@ ssize_t fgetxattrat_nofollow(int dirfd, const char *path, const char *name,
                              void *value, size_t size);
 int fsetxattrat_nofollow(int dirfd, const char *path, const char *name,
                          void *value, size_t size, int flags);
+ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
+                              char *list, size_t size);
+ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
+                                const char *name);
 
 #endif
diff --git a/hw/9pfs/9p-xattr.c b/hw/9pfs/9p-xattr.c
index d05c1a1c1df5..c696d8f8460f 100644
--- a/hw/9pfs/9p-xattr.c
+++ b/hw/9pfs/9p-xattr.c
@@ -60,17 +60,6 @@ ssize_t pt_listxattr(FsContext *ctx, const char *path,
     return name_size;
 }
 
-static ssize_t flistxattrat_nofollow(int dirfd, const char *filename,
-                                     char *list, size_t size)
-{
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-    int ret;
-
-    ret = llistxattr(proc_path, list, size);
-    g_free(proc_path);
-    return ret;
-}
-
 /*
  * Get the list and pass to each layer to find out whether
  * to send the data or not
@@ -196,17 +185,6 @@ ssize_t pt_getxattr(FsContext *ctx, const char *path, const char *name,
     return local_getxattr_nofollow(ctx, path, name, value, size);
 }
 
-int fsetxattrat_nofollow(int dirfd, const char *filename, const char *name,
-                         void *value, size_t size, int flags)
-{
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-    int ret;
-
-    ret = lsetxattr(proc_path, name, value, size, flags);
-    g_free(proc_path);
-    return ret;
-}
-
 ssize_t local_setxattr_nofollow(FsContext *ctx, const char *path,
                                 const char *name, void *value, size_t size,
                                 int flags)
@@ -235,17 +213,6 @@ int pt_setxattr(FsContext *ctx, const char *path, const char *name, void *value,
     return local_setxattr_nofollow(ctx, path, name, value, size, flags);
 }
 
-static ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
-                                       const char *name)
-{
-    char *proc_path = g_strdup_printf("/proc/self/fd/%d/%s", dirfd, filename);
-    int ret;
-
-    ret = lremovexattr(proc_path, name);
-    g_free(proc_path);
-    return ret;
-}
-
 ssize_t local_removexattr_nofollow(FsContext *ctx, const char *path,
                                    const char *name)
 {
-- 
2.14.4

[Qemu-devel] [PULL 4/7] 9p: xattr: Fix crashes due to free of uninitialized value

[Greg Kurz]

From: Keno Fischer <keno@juliacomputing.com>

If the size returned from llistxattr/lgetxattr is 0, we skipped
the malloc call, leaving xattr.value uninitialized. However, this
value is later passed to `g_free` without any further checks,
causing an error. Fix that by always calling g_malloc unconditionally.
If `size` is 0, it will return NULL, which is safe to pass to g_free.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d74302deeba5..4386d698177b 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3256,8 +3256,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
         xattr_fidp->fs.xattr.len = size;
         xattr_fidp->fid_type = P9_FID_XATTR;
         xattr_fidp->fs.xattr.xattrwalk_fid = true;
+        xattr_fidp->fs.xattr.value = g_malloc0(size);
         if (size) {
-            xattr_fidp->fs.xattr.value = g_malloc0(size);
             err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
                                      xattr_fidp->fs.xattr.value,
                                      xattr_fidp->fs.xattr.len);
@@ -3289,8 +3289,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
         xattr_fidp->fs.xattr.len = size;
         xattr_fidp->fid_type = P9_FID_XATTR;
         xattr_fidp->fs.xattr.xattrwalk_fid = true;
+        xattr_fidp->fs.xattr.value = g_malloc0(size);
         if (size) {
-            xattr_fidp->fs.xattr.value = g_malloc0(size);
             err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
                                     &name, xattr_fidp->fs.xattr.value,
                                     xattr_fidp->fs.xattr.len);
-- 
2.14.4

[Qemu-devel] [PULL 5/7] 9p: local: Avoid warning if FS_IOC_GETVERSION is not defined

[Greg Kurz]

From: Keno Fischer <keno@juliacomputing.com>

Both `stbuf` and `local_ioc_getversion` where unused when
FS_IOC_GETVERSION was not defined, causing a compiler warning.

Reorganize the code to avoid this warning.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 7758c3850900..5721eff1e189 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1373,10 +1373,10 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
     return ret;
 }
 
+#ifdef FS_IOC_GETVERSION
 static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
                                 mode_t st_mode, uint64_t *st_gen)
 {
-#ifdef FS_IOC_GETVERSION
     int err;
     V9fsFidOpenState fid_open;
 
@@ -1395,32 +1395,21 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
     err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
     local_close(ctx, &fid_open);
     return err;
-#else
-    errno = ENOTTY;
-    return -1;
-#endif
 }
+#endif
 
-static int local_init(FsContext *ctx, Error **errp)
+static int local_ioc_getversion_init(FsContext *ctx, LocalData *data, Error **errp)
 {
+#ifdef FS_IOC_GETVERSION
     struct statfs stbuf;
-    LocalData *data = g_malloc(sizeof(*data));
 
-    data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
-    if (data->mountfd == -1) {
-        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
-        goto err;
-    }
-
-#ifdef FS_IOC_GETVERSION
     /*
      * use ioc_getversion only if the ioctl is definied
      */
     if (fstatfs(data->mountfd, &stbuf) < 0) {
-        close_preserve_errno(data->mountfd);
         error_setg_errno(errp, errno,
-            "failed to stat file system at '%s'", ctx->fs_root);
-        goto err;
+                         "failed to stat file system at '%s'", ctx->fs_root);
+        return -1;
     }
     switch (stbuf.f_type) {
     case EXT2_SUPER_MAGIC:
@@ -1431,6 +1420,23 @@ static int local_init(FsContext *ctx, Error **errp)
         break;
     }
 #endif
+    return 0;
+}
+
+static int local_init(FsContext *ctx, Error **errp)
+{
+    LocalData *data = g_malloc(sizeof(*data));
+
+    data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
+    if (data->mountfd == -1) {
+        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
+        goto err;
+    }
+
+    if (local_ioc_getversion_init(ctx, data, errp) < 0) {
+        close(data->mountfd);
+        goto err;
+    }
 
     if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
         ctx->xops = passthrough_xattr_ops;
-- 
2.14.4

[Qemu-devel] [PULL 6/7] 9p: Properly check/translate flags in unlinkat

[Greg Kurz]

From: Keno Fischer <keno@juliacomputing.com>

The 9p-local code previously relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR
having the same numerical value and deferred any errorchecking to the
syscall itself. However, while the former assumption is true on Linux,
it is not true in general. 9p-handle did this properly however. Move
the translation code to the generic 9p server code and add an error
if unrecognized flags are passed.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-handle.c |  8 +-------
 hw/9pfs/9p.c        | 13 +++++++++++--
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index 4dc0d2bed1c1..f3641dbe4a92 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -559,19 +559,13 @@ static int handle_unlinkat(FsContext *ctx, V9fsPath *dir,
 {
     int dirfd, ret;
     HandleData *data = (HandleData *) ctx->private;
-    int rflags;
 
     dirfd = open_by_handle(data->mountfd, dir->data, O_PATH);
     if (dirfd < 0) {
         return dirfd;
     }
 
-    rflags = 0;
-    if (flags & P9_DOTL_AT_REMOVEDIR) {
-        rflags |= AT_REMOVEDIR;
-    }
-
-    ret = unlinkat(dirfd, name, rflags);
+    ret = unlinkat(dirfd, name, flags);
 
     close(dirfd);
     return ret;
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4386d698177b..c842ec555ea1 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2522,7 +2522,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 {
     int err = 0;
     V9fsString name;
-    int32_t dfid, flags;
+    int32_t dfid, flags, rflags = 0;
     size_t offset = 7;
     V9fsPath path;
     V9fsFidState *dfidp;
@@ -2549,6 +2549,15 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
         goto out_nofid;
     }
 
+    if (flags & ~P9_DOTL_AT_REMOVEDIR) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
+    if (flags & P9_DOTL_AT_REMOVEDIR) {
+        rflags |= AT_REMOVEDIR;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
@@ -2567,7 +2576,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
     if (err < 0) {
         goto out_err;
     }
-    err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, flags);
+    err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, rflags);
     if (!err) {
         err = offset;
     }
-- 
2.14.4

[Qemu-devel] [PULL 7/7] 9p: xattr: Properly translate xattrcreate flags

[Greg Kurz]

From: Keno Fischer <keno@juliacomputing.com>

As with unlinkat, these flags come from the client and need to
be translated to their host values. The protocol values happen
to match linux, but that need not be true in general.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 17 +++++++++++++++--
 hw/9pfs/9p.h |  4 ++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c842ec555ea1..eef289e394d4 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3327,7 +3327,7 @@ out_nofid:
 
 static void coroutine_fn v9fs_xattrcreate(void *opaque)
 {
-    int flags;
+    int flags, rflags = 0;
     int32_t fid;
     uint64_t size;
     ssize_t err = 0;
@@ -3344,6 +3344,19 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
     }
     trace_v9fs_xattrcreate(pdu->tag, pdu->id, fid, name.data, size, flags);
 
+    if (flags & ~(P9_XATTR_CREATE | P9_XATTR_REPLACE)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
+    if (flags & P9_XATTR_CREATE) {
+        rflags |= XATTR_CREATE;
+    }
+
+    if (flags & P9_XATTR_REPLACE) {
+        rflags |= XATTR_REPLACE;
+    }
+
     if (size > XATTR_SIZE_MAX) {
         err = -E2BIG;
         goto out_nofid;
@@ -3365,7 +3378,7 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
     xattr_fidp->fs.xattr.copied_len = 0;
     xattr_fidp->fs.xattr.xattrwalk_fid = false;
     xattr_fidp->fs.xattr.len = size;
-    xattr_fidp->fs.xattr.flags = flags;
+    xattr_fidp->fs.xattr.flags = rflags;
     v9fs_string_init(&xattr_fidp->fs.xattr.name);
     v9fs_string_copy(&xattr_fidp->fs.xattr.name, &name);
     xattr_fidp->fs.xattr.value = g_malloc0(size);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index bad8ee719c4b..8883761b2c1d 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -169,6 +169,10 @@ typedef struct V9fsConf
     char *fsdev_id;
 } V9fsConf;
 
+/* 9p2000.L xattr flags (matches Linux values) */
+#define P9_XATTR_CREATE 1
+#define P9_XATTR_REPLACE 2
+
 typedef struct V9fsXattr
 {
     uint64_t copied_len;
-- 
2.14.4

Re: [Qemu-devel] [PULL 0/7] 9p patches 2018-06-07

[Peter Maydell]

On 7 June 2018 at 16:21, Greg Kurz <groug@kaod.org> wrote:
> The following changes since commit 5d328d7d2f1fd4fb160bcfb6e4eb838720274438:
>
>   Merge remote-tracking branch 'remotes/awilliam/tags/vfio-update-20180605.0' into staging (2018-06-07 08:59:28 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/gkurz/qemu.git tags/for-upstream
>
> for you to fetch changes up to aca6897fba149a2a650dcdf5a5e1ae828371f4aa:
>
>   9p: xattr: Properly translate xattrcreate flags (2018-06-07 12:17:22 +0200)
>
> ----------------------------------------------------------------
> Mostly bug fixes and code sanitization motivated by the upcoming
> support for Darwin hosts. Thanks to Keno Fischer.
>
> ----------------------------------------------------------------
> Keno Fischer (7):
>       9p: proxy: Fix size passed to `connect`
>       9p: local: Properly set errp in fstatfs error path
>       9p: Move a couple xattr functions to 9p-util
>       9p: xattr: Fix crashes due to free of uninitialized value
>       9p: local: Avoid warning if FS_IOC_GETVERSION is not defined
>       9p: Properly check/translate flags in unlinkat
>       9p: xattr: Properly translate xattrcreate flags

Applied, thanks.

-- PMM