diff mbox

[RESEND-try-2] hw/9pfs: fix P9_STATS_GEN handling

Message ID 1385726178-10201-1-git-send-email-kirill.shutemov@linux.intel.com
State New
Headers show

Commit Message

Kirill A. Shutemov Nov. 29, 2013, 11:56 a.m. UTC 
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(-)

Comments

Kirill A. Shutemov Dec. 9, 2013, 10:25 a.m. UTC | #1 
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>

Ping?
Aneesh Kumar K.V Dec. 9, 2013, 11:26 a.m. UTC | #2 
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:

> 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>
>
> Ping?
>

I am hoping that this will go to upstream directly without me doing a
pull request ? Anthony, let me know if this need anything to be done
from my side


-aneesh
Kirill A. Shutemov Dec. 23, 2013, 12:21 p.m. UTC | #3 
Aneesh Kumar K.V wrote:
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
> 
> > 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>
> >
> > Ping?
> >
> 
> I am hoping that this will go to upstream directly without me doing a
> pull request ? Anthony, let me know if this need anything to be done
> from my side

So? Nobody cares?
Markus Armbruster Jan. 7, 2014, 10:12 a.m. UTC | #4 
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:

> Aneesh Kumar K.V wrote:
>> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> writes:
>> 
>> > 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>
>> >
>> > Ping?
>> >
>> 
>> I am hoping that this will go to upstream directly without me doing a
>> pull request ? Anthony, let me know if this need anything to be done
>> from my side
>
> So? Nobody cares?

Unlikely.  Your problem is that the "Anthony" route for patches has been
dug up, and approximately 95% of the traffic still trying to navigate it
fall into its holes and vanish without trace.  Recommend to try another
route.
diff mbox

Patch

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) {