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

2013-10-30 Thread Daniel P. Berrange 
On Sun, Oct 27, 2013 at 03:41:34AM +0300, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" 
> 
> 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 

Reviewed-by: Daniel P. Berrange 

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

2013-10-30 Thread Kirill A. Shutemov 
On Sun, Oct 27, 2013 at 03:41:34AM +0300, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" 
> 
> 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 

Ping?

-- 
 Kirill A. Shutemov

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

2013-10-26 Thread Kirill A. Shutemov 
From: "Kirill A. Shutemov" 

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 
---
 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 194c1306c6..2efebf3571 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 fe8e0ed19d..17002a3d28 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 fc93e9e6e8..df0dbffa7a 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 5f44bb758b..b57966d9d8 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 8cbb8ae32a..3e51fcd152 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 = p

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

2013-10-26 Thread Kirill A. Shutemov 
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 
---
 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 194c1306c6..2efebf3571 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 fe8e0ed19d..17002a3d28 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 fc93e9e6e8..df0dbffa7a 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 5f44bb758b..b57966d9d8 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 8cbb8ae32a..3e51fcd152 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",