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

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 28/01/2014 11:55, Kirill A. Shutemov ha scritto:
>> 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>
>> ---
>>  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;
>> +        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) {
>>
>
> Michael, are you going to take this patch given that the virtio-9p 
> maintainer is AWOL?

I did review the patch, and also specifically asked if we need a pull
request with this just one patch or can it be taken directly to upstream
tree. I was not sure whether generating a pull request with just one
patch was useful. 

-aneesh