Re: [Qemu-devel] [PATCH] [RESEND-try-3] hw/9pfs: fix P9_STATS_GEN handling

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 28, 2014 at 12:55:51PM +0200, Kirill A. Shutemov wrote:
> Currently we have few issues with P9_STATS_GEN:
> 
>  - We don't try to read st_gen anything except files or directories, but
>    still set P9_STATS_GEN bit in st_result_mask. It may mislead client:
>    we present garbage as valid st_gen.
> 
>  - If we failed to get valid st_gen with ENOTTY, we ignore error, but
>    still set P9_STATS_GEN bit in st_result_mask.
> 
>  - If we failed to get valid st_gen with any other errno, we fail
>    getattr altogether. It's excessive: we block valid client use-cases,
>    like chdir(2) to non-readable directory with execution bit set.
> 
> The patch fixes these issues and cleanup code a bit.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

Would be better to split unrelated issues out to separate patches.

> ---
>  hw/9pfs/cofile.c           |  4 ----
>  hw/9pfs/virtio-9p-handle.c |  8 +++++++-
>  hw/9pfs/virtio-9p-local.c  | 10 ++++++----
>  hw/9pfs/virtio-9p-proxy.c  |  3 ++-
>  hw/9pfs/virtio-9p.c        | 12 ++++++++++--
>  5 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
> index 194c1306c665..2efebf35710f 100644
> --- a/hw/9pfs/cofile.c
> +++ b/hw/9pfs/cofile.c
> @@ -38,10 +38,6 @@ int v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t st_mode,
>              });
>          v9fs_path_unlock(s);
>      }
> -    /* The ioctl may not be supported depending on the path */
> -    if (err == -ENOTTY) {
> -        err = 0;
> -    }
>      return err;
>  }
>  
> diff --git a/hw/9pfs/virtio-9p-handle.c b/hw/9pfs/virtio-9p-handle.c
> index fe8e0ed19dcc..17002a3d2867 100644
> --- a/hw/9pfs/virtio-9p-handle.c
> +++ b/hw/9pfs/virtio-9p-handle.c
> @@ -582,6 +582,7 @@ static int handle_unlinkat(FsContext *ctx, V9fsPath *dir,
>  static int handle_ioc_getversion(FsContext *ctx, V9fsPath *path,
>                                   mode_t st_mode, uint64_t *st_gen)
>  {
> +#ifdef FS_IOC_GETVERSION
>      int err;
>      V9fsFidOpenState fid_open;
>  
> @@ -590,7 +591,8 @@ static int handle_ioc_getversion(FsContext *ctx, V9fsPath *path,
>       * We can get fd for regular files and directories only
>       */
>      if (!S_ISREG(st_mode) && !S_ISDIR(st_mode)) {
> -            return 0;
> +        errno = ENOTTY;
> +        return -1;
>      }
>      err = handle_open(ctx, path, O_RDONLY, &fid_open);
>      if (err < 0) {
> @@ -599,6 +601,10 @@ static int handle_ioc_getversion(FsContext *ctx, V9fsPath *path,
>      err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
>      handle_close(ctx, &fid_open);
>      return err;
> +#else
> +    errno = ENOTTY;
> +    return -1;
> +#endif
>  }
>  
>  static int handle_init(FsContext *ctx)
> diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
> index fc93e9e6e8da..df0dbffa7ac4 100644
> --- a/hw/9pfs/virtio-9p-local.c
> +++ b/hw/9pfs/virtio-9p-local.c
> @@ -1068,8 +1068,8 @@ err_out:
>  static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
>                                  mode_t st_mode, uint64_t *st_gen)
>  {
> -    int err;
>  #ifdef FS_IOC_GETVERSION
> +    int err;
>      V9fsFidOpenState fid_open;
>  
>      /*
> @@ -1077,7 +1077,8 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
>       * We can get fd for regular files and directories only
>       */
>      if (!S_ISREG(st_mode) && !S_ISDIR(st_mode)) {
> -            return 0;
> +        errno = ENOTTY;
> +        return -1;
>      }
>      err = local_open(ctx, path, O_RDONLY, &fid_open);
>      if (err < 0) {
> @@ -1085,10 +1086,11 @@ 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
> -    err = -ENOTTY;
> +    errno = ENOTTY;
> +    return -1;
>  #endif
> -    return err;
>  }
>  
>  static int local_init(FsContext *ctx)
> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
> index 5f44bb758b35..b57966d9d883 100644
> --- a/hw/9pfs/virtio-9p-proxy.c
> +++ b/hw/9pfs/virtio-9p-proxy.c
> @@ -1086,7 +1086,8 @@ static int proxy_ioc_getversion(FsContext *fs_ctx, V9fsPath *path,
>       * we can get fd for regular files and directories only
>       */
>      if (!S_ISREG(st_mode) && !S_ISDIR(st_mode)) {
> -        return 0;
> +        errno = ENOTTY;
> +        return -1;
>      }
>      err = v9fs_request(fs_ctx->private, T_GETVERSION, st_gen, "s", path);
>      if (err < 0) {
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 8cbb8ae32a03..3e51fcd152f8 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -1080,10 +1080,18 @@ static void v9fs_getattr(void *opaque)
>      /*  fill st_gen if requested and supported by underlying fs */
>      if (request_mask & P9_STATS_GEN) {
>          retval = v9fs_co_st_gen(pdu, &fidp->path, stbuf.st_mode, &v9stat_dotl);
> -        if (retval < 0) {
> +        switch (retval) {
> +        case 0:
> +            /* we have valid st_gen: update result mask */
> +            v9stat_dotl.st_result_mask |= P9_STATS_GEN;
> +            break;
> +        case -EINTR:
> +            /* request cancelled */
>              goto out;

Shouldn't EINTR be retried?

> +        default:
> +            /* failed to get st_gen: not fatal, ignore */
> +            break;
>          }
> -        v9stat_dotl.st_result_mask |= P9_STATS_GEN;
>      }
>      retval = pdu_marshal(pdu, offset, "A", &v9stat_dotl);
>      if (retval < 0) {
> -- 
> 1.8.5.2
>