Re: [Qemu-devel] [PATCH v3 1/1] Add support for custom fmasks/dmasks in 9ps mapped mode

[Greg Kurz]

On Sun, 18 Jun 2017 18:17:27 +0200
Tobias Schramm <address@hidden> wrote:

Hi,

The patch titles are usually prefixed by the component name. Something like:

9pfs: local: add support for custom fmask/dmask in mapped security modes

Also, it is always nice to write a changelog, even if the feature looks simple.
At least, explain why we need that.

> Signed-off-by: Tobias Schramm <address@hidden>
> ---
>  v3: Use unsigned types for umask
>  v2: Adjust patch to QEMU code style
> 
>  fsdev/file-op-9p.h      |  4 ++++
>  fsdev/qemu-fsdev-opts.c | 12 ++++++++++++
>  hw/9pfs/9p-local.c      | 29 +++++++++++++++++++++++++----
>  hw/9pfs/9p.c            |  3 +++

This patch changes the command line. Please update the documentation
accordingly (qemu-options.hx).

>  4 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 0844a403dc..0ecb1d392b 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -76,6 +76,8 @@ typedef struct FsDriverEntry {
>      int export_flags;
>      FileOperations *ops;
>      FsThrottle fst;
> +    mode_t fmask;
> +    mode_t dmask;
>  } FsDriverEntry;
>  
>  typedef struct FsContext
> @@ -88,6 +90,8 @@ typedef struct FsContext
>      FsThrottle *fst;
>      /* fs driver specific data */
>      void *private;
> +    mode_t fmask;
> +    mode_t dmask;
>  } FsContext;
>  
>  typedef struct V9fsPath {
> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> index bf5713008a..dd6391edb0 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -38,6 +38,12 @@ static QemuOptsList qemu_fsdev_opts = {
>          }, {
>              .name = "sock_fd",
>              .type = QEMU_OPT_NUMBER,
> +        }, {
> +            .name = "fmask",
> +            .type = QEMU_OPT_STRING,

Why not using QEMU_OPT_NUMBER ?

> +        }, {
> +            .name = "dmask",
> +            .type = QEMU_OPT_STRING,
>          },
>  
>          THROTTLE_OPTS,
> @@ -75,6 +81,12 @@ static QemuOptsList qemu_virtfs_opts = {
>          }, {
>              .name = "sock_fd",
>              .type = QEMU_OPT_NUMBER,
> +        }, {
> +            .name = "fmask",
> +            .type = QEMU_OPT_STRING,
> +        }, {
> +            .name = "dmask",
> +            .type = QEMU_OPT_STRING,
>          },
>  
>          { /*End of list */ }
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 1e78b7c9e9..ef7282e294 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -633,7 +633,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath 
> *dir_path,
>  
>      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> -        err = mknodat(dirfd, name, SM_LOCAL_MODE_BITS | S_IFREG, 0);
> +        err = mknodat(dirfd, name, fs_ctx->fmask | S_IFREG, 0);
>          if (err == -1) {
>              goto out;
>          }
> @@ -685,7 +685,7 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath 
> *dir_path,
>  
>      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> -        err = mkdirat(dirfd, name, SM_LOCAL_DIR_MODE_BITS);
> +        err = mkdirat(dirfd, name, fs_ctx->dmask);
>          if (err == -1) {
>              goto out;
>          }
> @@ -786,7 +786,7 @@ static int local_open2(FsContext *fs_ctx, V9fsPath 
> *dir_path, const char *name,
>      /* Determine the security model */
>      if (fs_ctx->export_flags & V9FS_SM_MAPPED ||
>          fs_ctx->export_flags & V9FS_SM_MAPPED_FILE) {
> -        fd = openat_file(dirfd, name, flags, SM_LOCAL_MODE_BITS);
> +        fd = openat_file(dirfd, name, flags, fs_ctx->fmask);
>          if (fd == -1) {
>              goto out;
>          }
> @@ -849,7 +849,7 @@ static int local_symlink(FsContext *fs_ctx, const char 
> *oldpath,
>          ssize_t oldpath_size, write_size;
>  
>          fd = openat_file(dirfd, name, O_CREAT | O_EXCL | O_RDWR,
> -                         SM_LOCAL_MODE_BITS);
> +                         fs_ctx->fmask);
>          if (fd == -1) {
>              goto out;
>          }
> @@ -1431,6 +1431,9 @@ static int local_parse_opts(QemuOpts *opts, struct 
> FsDriverEntry *fse)
>  {
>      const char *sec_model = qemu_opt_get(opts, "security_model");
>      const char *path = qemu_opt_get(opts, "path");
> +    const char *fmask = qemu_opt_get(opts, "fmask");
> +    const char *dmask = qemu_opt_get(opts, "dmask");
> +    unsigned long mask;
>      Error *err = NULL;
>  
>      if (!sec_model) {
> @@ -1469,6 +1472,24 @@ static int local_parse_opts(QemuOpts *opts, struct 
> FsDriverEntry *fse)
>  
>      fse->path = g_strdup(path);
>  
> +    fse->fmask = SM_LOCAL_MODE_BITS;
> +    if (fmask) {
> +        if (qemu_strtoul(fmask, NULL, 0, &mask)) {
> +            error_report("Invalid fmask %s specified", fmask);
> +            return -1;

If the user passes an invalid fmask, then fse->path is leaked.

Sanity checks should happen before, or you have to free allocated resources.

In the present case, I guess the g_strdup() line should be the last thing this
function does.

> +        }
> +        fse->fmask = ((mode_t)mask) & 0777;
> +    }
> +
> +    fse->dmask = SM_LOCAL_DIR_MODE_BITS;
> +    if (dmask) {
> +        if (qemu_strtoul(dmask, NULL, 0, &mask)) {
> +            error_report("Invalid dmask %s specified", dmask);
> +            return -1;
> +        }
> +        fse->dmask = ((mode_t)mask) & 0777;
> +    }
> +

All of this is for mapped modes only. It should fail and print an error when
used with other modes.

>      return 0;
>  }
>  
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 96d2683348..40290dbade 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3533,6 +3533,9 @@ int v9fs_device_realize_common(V9fsState *s, Error 
> **errp)
>  
>      s->ops = fse->ops;
>  
> +    s->ctx.fmask = fse->fmask;
> +    s->ctx.dmask = fse->dmask;
> +
>      s->fid_list = NULL;
>      qemu_co_rwlock_init(&s->rename_lock);
>

pgpWBE7WEqrQL.pgp
Description: OpenPGP digital signature