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

2013-11-06 Thread Aneesh Kumar K.V 
Kirill A. Shutemov kir...@shutemov.name writes:

 From: Kirill A. Shutemov kirill.shute...@linux.intel.com

 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.

We should return 0 right ? We do 

memset(v9lstat, 0, sizeof(*v9lstat));

in stat_to_v9stat_dotl


  - If we failed to get valid st_gen with ENOTTY, we ignore error, but
still set P9_STATS_GEN bit in st_result_mask.

and have v9lstat.st_gen set to zero 


  - 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.


Can you explain this in detail ? 

 The patch fixes these issues and cleanup code a bit.

 Signed-off-by: Kirill A. Shutemov kirill.shute...@linux.intel.com
 Reviewed-by: Daniel P. Berrange berra...@redhat.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(-)


-aneesh

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

2013-11-06 Thread Kirill A. Shutemov 
On Wed, Nov 06, 2013 at 09:41:47PM +0530, Aneesh Kumar K.V wrote:
 Kirill A. Shutemov kir...@shutemov.name writes:
 
  From: Kirill A. Shutemov kirill.shute...@linux.intel.com
 
  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.
 
 We should return 0 right ? We do 
 
 memset(v9lstat, 0, sizeof(*v9lstat));
 
 in stat_to_v9stat_dotl

The right way is not set P9_STATS_GEN in st_result_mask if we don't know
it.

   - If we failed to get valid st_gen with ENOTTY, we ignore error, but
 still set P9_STATS_GEN bit in st_result_mask.
 
 and have v9lstat.st_gen set to zero 

The same as above.

And if ioctl(fd, FS_IOC_GETVERSION, st_gen) fails, nobody specifies what
will be stored into st_gen, if any. We should not relay that fs will not
touch st_gen even if it sounds reasonable.

 
 
   - 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.
 
 
 Can you explain this in detail ? 

If you have following tree:
 
% mkdir testdir
% echo test  testdir/testfile
% chmod -r testdir
 
In normal environment it's usable: you can chdir(2) into it and read files
inside if you know its name:
 
% cd testdir
% cat testfile
test
 
You only can't list directory content:
 
% ls
ls: cannot open directory .: Permission denied
 
With current qemu 9p implementation you'll get on guest -EACCES on chdir(2)
or read, since qemu fill fail to provide basic stats to guess. It happens
because qemu try open(2) non-readable file to run FS_IOC_GETVERSION and
fails getattr altogether.
 
I believe it also breaks more trivial use-cases: ls -l on non-readable
file or directory for the same reason. But I haven't checked that.

-- 
 Kirill A. Shutemov

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

2013-11-06 Thread Aneesh Kumar K.V 
Kirill A. Shutemov kir...@shutemov.name writes:

 From: Kirill A. Shutemov kirill.shute...@linux.intel.com

 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.shute...@linux.intel.com
 Reviewed-by: Daniel P. Berrange berra...@redhat.com

Reviewed-by: Aneesh Kumar K.V aneesh.ku...@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

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

2013-11-04 Thread Kirill A. Shutemov 
From: Kirill A. Shutemov kirill.shute...@linux.intel.com

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.shute...@linux.intel.com
Reviewed-by: Daniel P. Berrange berra...@redhat.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,