[Virtio-fs] [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Virtio-fs] [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Vivek Goyal]

Laszlo is writing a virtiofs client for OVMF and noticed that if he
sends fuse FLUSH command for directory object, virtiofsd crashes.
virtiofsd does not expect a FLUSH arriving for a directory object.

This patch series has one of the patches which fixes that. It also
has couple of posix lock fixes as a result of lo_flush() related debugging.

Vivek Goyal (3):
  virtiofsd: Set up posix_lock hash table for root inode
  virtiofsd: Disable posix_lock hash table if remote locks are not
    enabled
  virtiofsd: Check file type in lo_flush()

 tools/virtiofsd/passthrough_ll.c | 54 +++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 15 deletions(-)

-- 
2.25.4

[PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Vivek Goyal]

Laszlo is writing a virtiofs client for OVMF and noticed that if he
sends fuse FLUSH command for directory object, virtiofsd crashes.
virtiofsd does not expect a FLUSH arriving for a directory object.

This patch series has one of the patches which fixes that. It also
has couple of posix lock fixes as a result of lo_flush() related debugging.

Vivek Goyal (3):
  virtiofsd: Set up posix_lock hash table for root inode
  virtiofsd: Disable posix_lock hash table if remote locks are not
    enabled
  virtiofsd: Check file type in lo_flush()

 tools/virtiofsd/passthrough_ll.c | 54 +++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 15 deletions(-)

-- 
2.25.4

[Virtio-fs] [PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode

[Vivek Goyal]

We setup per inode hash table ->posix_lock to support remote posix locks.
But we forgot to initialize this table for root inode.

Laszlo managed to trigger an issue where he sent a FUSE_FLUSH request for
root inode and lo_flush() found inode with inode->posix_lock NULL and
accessing this table crashed virtiofsd.

May be we can get rid of initializing this hash table for directory
objects completely. But that optimization is for another day.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 97485b22b4..59202a843b 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3372,6 +3372,9 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
     root->key.mnt_id = mnt_id;
     root->nlookup = 2;
     g_atomic_int_set(&root->refcount, 2);
+    pthread_mutex_init(&root->plock_mutex, NULL);
+    root->posix_locks = g_hash_table_new_full(
+        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
 }
 
 static guint lo_key_hash(gconstpointer key)
@@ -3394,6 +3397,10 @@ static void fuse_lo_data_cleanup(struct lo_data *lo)
     if (lo->inodes) {
         g_hash_table_destroy(lo->inodes);
     }
+
+    if (lo->root.posix_locks)
+       g_hash_table_destroy(lo->root.posix_locks);
+
     lo_map_destroy(&lo->fd_map);
     lo_map_destroy(&lo->dirp_map);
     lo_map_destroy(&lo->ino_map);
-- 
2.25.4

[PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode

[Vivek Goyal]

We setup per inode hash table ->posix_lock to support remote posix locks.
But we forgot to initialize this table for root inode.

Laszlo managed to trigger an issue where he sent a FUSE_FLUSH request for
root inode and lo_flush() found inode with inode->posix_lock NULL and
accessing this table crashed virtiofsd.

May be we can get rid of initializing this hash table for directory
objects completely. But that optimization is for another day.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 97485b22b4..59202a843b 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -3372,6 +3372,9 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
     root->key.mnt_id = mnt_id;
     root->nlookup = 2;
     g_atomic_int_set(&root->refcount, 2);
+    pthread_mutex_init(&root->plock_mutex, NULL);
+    root->posix_locks = g_hash_table_new_full(
+        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
 }
 
 static guint lo_key_hash(gconstpointer key)
@@ -3394,6 +3397,10 @@ static void fuse_lo_data_cleanup(struct lo_data *lo)
     if (lo->inodes) {
         g_hash_table_destroy(lo->inodes);
     }
+
+    if (lo->root.posix_locks)
+       g_hash_table_destroy(lo->root.posix_locks);
+
     lo_map_destroy(&lo->fd_map);
     lo_map_destroy(&lo->dirp_map);
     lo_map_destroy(&lo->ino_map);
-- 
2.25.4

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode

[Vivek Goyal]

We setup per inode hash table ->posix_lock to support remote posix locks.
But we forgot to initialize this table for root inode.

Laszlo managed to trigger an issue where he sent a FUSE_FLUSH request for
root inode and lo_flush() found inode with inode->posix_lock NULL and
accessing this table crashed virtiofsd.

May be we can get rid of initializing this hash table for directory
objects completely. But that optimization is for another day.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:46:22.198359486 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:48:07.873737472 -0500
@@ -3372,6 +3372,9 @@ static void setup_root(struct lo_data *l
     root->key.mnt_id = mnt_id;
     root->nlookup = 2;
     g_atomic_int_set(&root->refcount, 2);
+    pthread_mutex_init(&root->plock_mutex, NULL);
+    root->posix_locks = g_hash_table_new_full(
+        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
 }
 
 static guint lo_key_hash(gconstpointer key)
@@ -3394,6 +3397,10 @@ static void fuse_lo_data_cleanup(struct
     if (lo->inodes) {
         g_hash_table_destroy(lo->inodes);
     }
+
+    if (lo->root.posix_locks) {
+        g_hash_table_destroy(lo->root.posix_locks);
+    }
     lo_map_destroy(&lo->fd_map);
     lo_map_destroy(&lo->dirp_map);
     lo_map_destroy(&lo->ino_map);

Re: [PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode

[Vivek Goyal]

We setup per inode hash table ->posix_lock to support remote posix locks.
But we forgot to initialize this table for root inode.

Laszlo managed to trigger an issue where he sent a FUSE_FLUSH request for
root inode and lo_flush() found inode with inode->posix_lock NULL and
accessing this table crashed virtiofsd.

May be we can get rid of initializing this hash table for directory
objects completely. But that optimization is for another day.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:46:22.198359486 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:48:07.873737472 -0500
@@ -3372,6 +3372,9 @@ static void setup_root(struct lo_data *l
     root->key.mnt_id = mnt_id;
     root->nlookup = 2;
     g_atomic_int_set(&root->refcount, 2);
+    pthread_mutex_init(&root->plock_mutex, NULL);
+    root->posix_locks = g_hash_table_new_full(
+        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
 }
 
 static guint lo_key_hash(gconstpointer key)
@@ -3394,6 +3397,10 @@ static void fuse_lo_data_cleanup(struct
     if (lo->inodes) {
         g_hash_table_destroy(lo->inodes);
     }
+
+    if (lo->root.posix_locks) {
+        g_hash_table_destroy(lo->root.posix_locks);
+    }
     lo_map_destroy(&lo->fd_map);
     lo_map_destroy(&lo->dirp_map);
     lo_map_destroy(&lo->ino_map);

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> We setup per inode hash table ->posix_lock to support remote posix locks.
> But we forgot to initialize this table for root inode.
> 
> Laszlo managed to trigger an issue where he sent a FUSE_FLUSH request for
> root inode and lo_flush() found inode with inode->posix_lock NULL and
> accessing this table crashed virtiofsd.
> 
> May be we can get rid of initializing this hash table for directory
> objects completely. But that optimization is for another day.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:46:22.198359486 -0500
> +++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:48:07.873737472 -0500
> @@ -3372,6 +3372,9 @@ static void setup_root(struct lo_data *l
>      root->key.mnt_id = mnt_id;
>      root->nlookup = 2;
>      g_atomic_int_set(&root->refcount, 2);
> +    pthread_mutex_init(&root->plock_mutex, NULL);
> +    root->posix_locks = g_hash_table_new_full(
> +        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
>  }
>  
>  static guint lo_key_hash(gconstpointer key)
> @@ -3394,6 +3397,10 @@ static void fuse_lo_data_cleanup(struct
>      if (lo->inodes) {
>          g_hash_table_destroy(lo->inodes);
>      }
> +
> +    if (lo->root.posix_locks) {
> +        g_hash_table_destroy(lo->root.posix_locks);
> +    }
>      lo_map_destroy(&lo->fd_map);
>      lo_map_destroy(&lo->dirp_map);
>      lo_map_destroy(&lo->ino_map);
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/3] virtiofsd: Set up posix_lock hash table for root inode

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> We setup per inode hash table ->posix_lock to support remote posix locks.
> But we forgot to initialize this table for root inode.
> 
> Laszlo managed to trigger an issue where he sent a FUSE_FLUSH request for
> root inode and lo_flush() found inode with inode->posix_lock NULL and
> accessing this table crashed virtiofsd.
> 
> May be we can get rid of initializing this hash table for directory
> objects completely. But that optimization is for another day.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:46:22.198359486 -0500
> +++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-07 14:48:07.873737472 -0500
> @@ -3372,6 +3372,9 @@ static void setup_root(struct lo_data *l
>      root->key.mnt_id = mnt_id;
>      root->nlookup = 2;
>      g_atomic_int_set(&root->refcount, 2);
> +    pthread_mutex_init(&root->plock_mutex, NULL);
> +    root->posix_locks = g_hash_table_new_full(
> +        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
>  }
>  
>  static guint lo_key_hash(gconstpointer key)
> @@ -3394,6 +3397,10 @@ static void fuse_lo_data_cleanup(struct
>      if (lo->inodes) {
>          g_hash_table_destroy(lo->inodes);
>      }
> +
> +    if (lo->root.posix_locks) {
> +        g_hash_table_destroy(lo->root.posix_locks);
> +    }
>      lo_map_destroy(&lo->fd_map);
>      lo_map_destroy(&lo->dirp_map);
>      lo_map_destroy(&lo->ino_map);
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Virtio-fs] [PATCH 2/3] virtiofsd: Disable posix_lock hash table if remote locks are not enabled

[Vivek Goyal]

If remote posix locks are not enabled (lo->posix_lock == false), then disable
code paths taken to initialize inode->posix_lock hash table and corresponding
destruction and search etc.

lo_getlk() and lo_setlk() have been modified to return ENOSYS if daemon
does not support posix lock but client still sends a lock/unlock request.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 51 +++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 59202a843b..8ba79f503a 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -914,10 +914,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
         inode->key.ino = e->attr.st_ino;
         inode->key.dev = e->attr.st_dev;
         inode->key.mnt_id = mnt_id;
-        pthread_mutex_init(&inode->plock_mutex, NULL);
-        inode->posix_locks = g_hash_table_new_full(
-            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
-
+        if (lo->posix_lock) {
+            pthread_mutex_init(&inode->plock_mutex, NULL);
+            inode->posix_locks = g_hash_table_new_full(
+                g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+        }
         pthread_mutex_lock(&lo->mutex);
         inode->fuse_ino = lo_add_inode_mapping(req, inode);
         g_hash_table_insert(lo->inodes, &inode->key, inode);
@@ -1303,12 +1304,13 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
     if (!inode->nlookup) {
         lo_map_remove(&lo->ino_map, inode->fuse_ino);
         g_hash_table_remove(lo->inodes, &inode->key);
-        if (g_hash_table_size(inode->posix_locks)) {
-            fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
+        if (lo->posix_lock) {
+            if (g_hash_table_size(inode->posix_locks)) {
+                fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
+            }
+            g_hash_table_destroy(inode->posix_locks);
+            pthread_mutex_destroy(&inode->plock_mutex);
         }
-        g_hash_table_destroy(inode->posix_locks);
-        pthread_mutex_destroy(&inode->plock_mutex);
-
         /* Drop our refcount from lo_do_lookup() */
         lo_inode_put(lo, &inode);
     }
@@ -1784,6 +1786,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
              ino, fi->flags, fi->lock_owner, lock->l_type, lock->l_start,
              lock->l_len);
 
+    if (!lo->posix_lock) {
+        fuse_reply_err(req, ENOSYS);
+        return;
+    }
+
     inode = lo_inode(req, ino);
     if (!inode) {
         fuse_reply_err(req, EBADF);
@@ -1829,6 +1836,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
              ino, fi->flags, lock->l_type, lock->l_pid, fi->lock_owner, sleep,
              lock->l_whence, lock->l_start, lock->l_len);
 
+    if (!lo->posix_lock) {
+        fuse_reply_err(req, ENOSYS);
+        return;
+    }
+
     if (sleep) {
         fuse_reply_err(req, EOPNOTSUPP);
         return;
@@ -1953,6 +1965,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
     int res;
     (void)ino;
     struct lo_inode *inode;
+    struct lo_data *lo = lo_data(req);
 
     inode = lo_inode(req, ino);
     if (!inode) {
@@ -1961,12 +1974,14 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
     }
 
     /* An fd is going away. Cleanup associated posix locks */
-    pthread_mutex_lock(&inode->plock_mutex);
-    g_hash_table_remove(inode->posix_locks, GUINT_TO_POINTER(fi->lock_owner));
-    pthread_mutex_unlock(&inode->plock_mutex);
-
+    if (lo->posix_lock) {
+        pthread_mutex_lock(&inode->plock_mutex);
+        g_hash_table_remove(inode->posix_locks,
+            GUINT_TO_POINTER(fi->lock_owner));
+        pthread_mutex_unlock(&inode->plock_mutex);
+    }
     res = close(dup(lo_fi_fd(req, fi)));
-    lo_inode_put(lo_data(req), &inode);
+    lo_inode_put(lo, &inode);
     fuse_reply_err(req, res == -1 ? errno : 0);
 }
 
@@ -3372,9 +3387,11 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
     root->key.mnt_id = mnt_id;
     root->nlookup = 2;
     g_atomic_int_set(&root->refcount, 2);
-    pthread_mutex_init(&root->plock_mutex, NULL);
-    root->posix_locks = g_hash_table_new_full(
-        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+    if (lo->posix_lock) {
+        pthread_mutex_init(&root->plock_mutex, NULL);
+        root->posix_locks = g_hash_table_new_full(
+            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+    }
 }
 
 static guint lo_key_hash(gconstpointer key)
-- 
2.25.4

[PATCH 2/3] virtiofsd: Disable posix_lock hash table if remote locks are not enabled

[Vivek Goyal]

If remote posix locks are not enabled (lo->posix_lock == false), then disable
code paths taken to initialize inode->posix_lock hash table and corresponding
destruction and search etc.

lo_getlk() and lo_setlk() have been modified to return ENOSYS if daemon
does not support posix lock but client still sends a lock/unlock request.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 51 +++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 59202a843b..8ba79f503a 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -914,10 +914,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
         inode->key.ino = e->attr.st_ino;
         inode->key.dev = e->attr.st_dev;
         inode->key.mnt_id = mnt_id;
-        pthread_mutex_init(&inode->plock_mutex, NULL);
-        inode->posix_locks = g_hash_table_new_full(
-            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
-
+        if (lo->posix_lock) {
+            pthread_mutex_init(&inode->plock_mutex, NULL);
+            inode->posix_locks = g_hash_table_new_full(
+                g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+        }
         pthread_mutex_lock(&lo->mutex);
         inode->fuse_ino = lo_add_inode_mapping(req, inode);
         g_hash_table_insert(lo->inodes, &inode->key, inode);
@@ -1303,12 +1304,13 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
     if (!inode->nlookup) {
         lo_map_remove(&lo->ino_map, inode->fuse_ino);
         g_hash_table_remove(lo->inodes, &inode->key);
-        if (g_hash_table_size(inode->posix_locks)) {
-            fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
+        if (lo->posix_lock) {
+            if (g_hash_table_size(inode->posix_locks)) {
+                fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
+            }
+            g_hash_table_destroy(inode->posix_locks);
+            pthread_mutex_destroy(&inode->plock_mutex);
         }
-        g_hash_table_destroy(inode->posix_locks);
-        pthread_mutex_destroy(&inode->plock_mutex);
-
         /* Drop our refcount from lo_do_lookup() */
         lo_inode_put(lo, &inode);
     }
@@ -1784,6 +1786,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
              ino, fi->flags, fi->lock_owner, lock->l_type, lock->l_start,
              lock->l_len);
 
+    if (!lo->posix_lock) {
+        fuse_reply_err(req, ENOSYS);
+        return;
+    }
+
     inode = lo_inode(req, ino);
     if (!inode) {
         fuse_reply_err(req, EBADF);
@@ -1829,6 +1836,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
              ino, fi->flags, lock->l_type, lock->l_pid, fi->lock_owner, sleep,
              lock->l_whence, lock->l_start, lock->l_len);
 
+    if (!lo->posix_lock) {
+        fuse_reply_err(req, ENOSYS);
+        return;
+    }
+
     if (sleep) {
         fuse_reply_err(req, EOPNOTSUPP);
         return;
@@ -1953,6 +1965,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
     int res;
     (void)ino;
     struct lo_inode *inode;
+    struct lo_data *lo = lo_data(req);
 
     inode = lo_inode(req, ino);
     if (!inode) {
@@ -1961,12 +1974,14 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
     }
 
     /* An fd is going away. Cleanup associated posix locks */
-    pthread_mutex_lock(&inode->plock_mutex);
-    g_hash_table_remove(inode->posix_locks, GUINT_TO_POINTER(fi->lock_owner));
-    pthread_mutex_unlock(&inode->plock_mutex);
-
+    if (lo->posix_lock) {
+        pthread_mutex_lock(&inode->plock_mutex);
+        g_hash_table_remove(inode->posix_locks,
+            GUINT_TO_POINTER(fi->lock_owner));
+        pthread_mutex_unlock(&inode->plock_mutex);
+    }
     res = close(dup(lo_fi_fd(req, fi)));
-    lo_inode_put(lo_data(req), &inode);
+    lo_inode_put(lo, &inode);
     fuse_reply_err(req, res == -1 ? errno : 0);
 }
 
@@ -3372,9 +3387,11 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
     root->key.mnt_id = mnt_id;
     root->nlookup = 2;
     g_atomic_int_set(&root->refcount, 2);
-    pthread_mutex_init(&root->plock_mutex, NULL);
-    root->posix_locks = g_hash_table_new_full(
-        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+    if (lo->posix_lock) {
+        pthread_mutex_init(&root->plock_mutex, NULL);
+        root->posix_locks = g_hash_table_new_full(
+            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
+    }
 }
 
 static guint lo_key_hash(gconstpointer key)
-- 
2.25.4

Re: [Virtio-fs] [PATCH 2/3] virtiofsd: Disable posix_lock hash table if remote locks are not enabled

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> If remote posix locks are not enabled (lo->posix_lock == false), then disable
> code paths taken to initialize inode->posix_lock hash table and corresponding
> destruction and search etc.
> 
> lo_getlk() and lo_setlk() have been modified to return ENOSYS if daemon
> does not support posix lock but client still sends a lock/unlock request.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 51 +++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 59202a843b..8ba79f503a 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -914,10 +914,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>          inode->key.ino = e->attr.st_ino;
>          inode->key.dev = e->attr.st_dev;
>          inode->key.mnt_id = mnt_id;
> -        pthread_mutex_init(&inode->plock_mutex, NULL);
> -        inode->posix_locks = g_hash_table_new_full(
> -            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> -
> +        if (lo->posix_lock) {
> +            pthread_mutex_init(&inode->plock_mutex, NULL);
> +            inode->posix_locks = g_hash_table_new_full(
> +                g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> +        }
>          pthread_mutex_lock(&lo->mutex);
>          inode->fuse_ino = lo_add_inode_mapping(req, inode);
>          g_hash_table_insert(lo->inodes, &inode->key, inode);
> @@ -1303,12 +1304,13 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
>      if (!inode->nlookup) {
>          lo_map_remove(&lo->ino_map, inode->fuse_ino);
>          g_hash_table_remove(lo->inodes, &inode->key);
> -        if (g_hash_table_size(inode->posix_locks)) {
> -            fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
> +        if (lo->posix_lock) {
> +            if (g_hash_table_size(inode->posix_locks)) {
> +                fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
> +            }
> +            g_hash_table_destroy(inode->posix_locks);
> +            pthread_mutex_destroy(&inode->plock_mutex);
>          }
> -        g_hash_table_destroy(inode->posix_locks);
> -        pthread_mutex_destroy(&inode->plock_mutex);
> -
>          /* Drop our refcount from lo_do_lookup() */
>          lo_inode_put(lo, &inode);
>      }
> @@ -1784,6 +1786,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
>               ino, fi->flags, fi->lock_owner, lock->l_type, lock->l_start,
>               lock->l_len);
>  
> +    if (!lo->posix_lock) {
> +        fuse_reply_err(req, ENOSYS);
> +        return;
> +    }
> +
>      inode = lo_inode(req, ino);
>      if (!inode) {
>          fuse_reply_err(req, EBADF);
> @@ -1829,6 +1836,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
>               ino, fi->flags, lock->l_type, lock->l_pid, fi->lock_owner, sleep,
>               lock->l_whence, lock->l_start, lock->l_len);
>  
> +    if (!lo->posix_lock) {
> +        fuse_reply_err(req, ENOSYS);
> +        return;
> +    }
> +
>      if (sleep) {
>          fuse_reply_err(req, EOPNOTSUPP);
>          return;
> @@ -1953,6 +1965,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      int res;
>      (void)ino;
>      struct lo_inode *inode;
> +    struct lo_data *lo = lo_data(req);
>  
>      inode = lo_inode(req, ino);
>      if (!inode) {
> @@ -1961,12 +1974,14 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      }
>  
>      /* An fd is going away. Cleanup associated posix locks */
> -    pthread_mutex_lock(&inode->plock_mutex);
> -    g_hash_table_remove(inode->posix_locks, GUINT_TO_POINTER(fi->lock_owner));
> -    pthread_mutex_unlock(&inode->plock_mutex);
> -
> +    if (lo->posix_lock) {
> +        pthread_mutex_lock(&inode->plock_mutex);
> +        g_hash_table_remove(inode->posix_locks,
> +            GUINT_TO_POINTER(fi->lock_owner));
> +        pthread_mutex_unlock(&inode->plock_mutex);
> +    }
>      res = close(dup(lo_fi_fd(req, fi)));
> -    lo_inode_put(lo_data(req), &inode);
> +    lo_inode_put(lo, &inode);
>      fuse_reply_err(req, res == -1 ? errno : 0);
>  }
>  
> @@ -3372,9 +3387,11 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
>      root->key.mnt_id = mnt_id;
>      root->nlookup = 2;
>      g_atomic_int_set(&root->refcount, 2);
> -    pthread_mutex_init(&root->plock_mutex, NULL);
> -    root->posix_locks = g_hash_table_new_full(
> -        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> +    if (lo->posix_lock) {
> +        pthread_mutex_init(&root->plock_mutex, NULL);
> +        root->posix_locks = g_hash_table_new_full(
> +            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> +    }
>  }
>  
>  static guint lo_key_hash(gconstpointer key)
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/3] virtiofsd: Disable posix_lock hash table if remote locks are not enabled

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> If remote posix locks are not enabled (lo->posix_lock == false), then disable
> code paths taken to initialize inode->posix_lock hash table and corresponding
> destruction and search etc.
> 
> lo_getlk() and lo_setlk() have been modified to return ENOSYS if daemon
> does not support posix lock but client still sends a lock/unlock request.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tools/virtiofsd/passthrough_ll.c | 51 +++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 59202a843b..8ba79f503a 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -914,10 +914,11 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
>          inode->key.ino = e->attr.st_ino;
>          inode->key.dev = e->attr.st_dev;
>          inode->key.mnt_id = mnt_id;
> -        pthread_mutex_init(&inode->plock_mutex, NULL);
> -        inode->posix_locks = g_hash_table_new_full(
> -            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> -
> +        if (lo->posix_lock) {
> +            pthread_mutex_init(&inode->plock_mutex, NULL);
> +            inode->posix_locks = g_hash_table_new_full(
> +                g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> +        }
>          pthread_mutex_lock(&lo->mutex);
>          inode->fuse_ino = lo_add_inode_mapping(req, inode);
>          g_hash_table_insert(lo->inodes, &inode->key, inode);
> @@ -1303,12 +1304,13 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
>      if (!inode->nlookup) {
>          lo_map_remove(&lo->ino_map, inode->fuse_ino);
>          g_hash_table_remove(lo->inodes, &inode->key);
> -        if (g_hash_table_size(inode->posix_locks)) {
> -            fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
> +        if (lo->posix_lock) {
> +            if (g_hash_table_size(inode->posix_locks)) {
> +                fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
> +            }
> +            g_hash_table_destroy(inode->posix_locks);
> +            pthread_mutex_destroy(&inode->plock_mutex);
>          }
> -        g_hash_table_destroy(inode->posix_locks);
> -        pthread_mutex_destroy(&inode->plock_mutex);
> -
>          /* Drop our refcount from lo_do_lookup() */
>          lo_inode_put(lo, &inode);
>      }
> @@ -1784,6 +1786,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
>               ino, fi->flags, fi->lock_owner, lock->l_type, lock->l_start,
>               lock->l_len);
>  
> +    if (!lo->posix_lock) {
> +        fuse_reply_err(req, ENOSYS);
> +        return;
> +    }
> +
>      inode = lo_inode(req, ino);
>      if (!inode) {
>          fuse_reply_err(req, EBADF);
> @@ -1829,6 +1836,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi,
>               ino, fi->flags, lock->l_type, lock->l_pid, fi->lock_owner, sleep,
>               lock->l_whence, lock->l_start, lock->l_len);
>  
> +    if (!lo->posix_lock) {
> +        fuse_reply_err(req, ENOSYS);
> +        return;
> +    }
> +
>      if (sleep) {
>          fuse_reply_err(req, EOPNOTSUPP);
>          return;
> @@ -1953,6 +1965,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      int res;
>      (void)ino;
>      struct lo_inode *inode;
> +    struct lo_data *lo = lo_data(req);
>  
>      inode = lo_inode(req, ino);
>      if (!inode) {
> @@ -1961,12 +1974,14 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      }
>  
>      /* An fd is going away. Cleanup associated posix locks */
> -    pthread_mutex_lock(&inode->plock_mutex);
> -    g_hash_table_remove(inode->posix_locks, GUINT_TO_POINTER(fi->lock_owner));
> -    pthread_mutex_unlock(&inode->plock_mutex);
> -
> +    if (lo->posix_lock) {
> +        pthread_mutex_lock(&inode->plock_mutex);
> +        g_hash_table_remove(inode->posix_locks,
> +            GUINT_TO_POINTER(fi->lock_owner));
> +        pthread_mutex_unlock(&inode->plock_mutex);
> +    }
>      res = close(dup(lo_fi_fd(req, fi)));
> -    lo_inode_put(lo_data(req), &inode);
> +    lo_inode_put(lo, &inode);
>      fuse_reply_err(req, res == -1 ? errno : 0);
>  }
>  
> @@ -3372,9 +3387,11 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root)
>      root->key.mnt_id = mnt_id;
>      root->nlookup = 2;
>      g_atomic_int_set(&root->refcount, 2);
> -    pthread_mutex_init(&root->plock_mutex, NULL);
> -    root->posix_locks = g_hash_table_new_full(
> -        g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> +    if (lo->posix_lock) {
> +        pthread_mutex_init(&root->plock_mutex, NULL);
> +        root->posix_locks = g_hash_table_new_full(
> +            g_direct_hash, g_direct_equal, NULL, posix_locks_value_destroy);
> +    }
>  }
>  
>  static guint lo_key_hash(gconstpointer key)
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Virtio-fs] [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Vivek Goyal]

Currently lo_flush() is written in such a way that it expects to receive
a FLUSH requests on a regular file (and not directories). For example,
we call lo_fi_fd() which searches lo->fd_map. If we open directories
using opendir(), we keep don't keep track of these in lo->fd_map instead
we keep them in lo->dir_map. So we expect lo_flush() to be called on
regular files only.

Even linux fuse client calls FLUSH only for regular files and not
directories. So put a check for filetype and return EBADF if
lo_flush() is called on a non-regular file.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 8ba79f503a..48a109d3f6 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
     struct lo_data *lo = lo_data(req);
 
     inode = lo_inode(req, ino);
-    if (!inode) {
+    if (!inode || !S_ISREG(inode->filetype)) {
         fuse_reply_err(req, EBADF);
         return;
     }
-- 
2.25.4

[PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Vivek Goyal]

Currently lo_flush() is written in such a way that it expects to receive
a FLUSH requests on a regular file (and not directories). For example,
we call lo_fi_fd() which searches lo->fd_map. If we open directories
using opendir(), we keep don't keep track of these in lo->fd_map instead
we keep them in lo->dir_map. So we expect lo_flush() to be called on
regular files only.

Even linux fuse client calls FLUSH only for regular files and not
directories. So put a check for filetype and return EBADF if
lo_flush() is called on a non-regular file.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 8ba79f503a..48a109d3f6 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
     struct lo_data *lo = lo_data(req);
 
     inode = lo_inode(req, ino);
-    if (!inode) {
+    if (!inode || !S_ISREG(inode->filetype)) {
         fuse_reply_err(req, EBADF);
         return;
     }
-- 
2.25.4

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Currently lo_flush() is written in such a way that it expects to receive
> a FLUSH requests on a regular file (and not directories). For example,
> we call lo_fi_fd() which searches lo->fd_map. If we open directories
> using opendir(), we keep don't keep track of these in lo->fd_map instead
> we keep them in lo->dir_map. So we expect lo_flush() to be called on
> regular files only.
> 
> Even linux fuse client calls FLUSH only for regular files and not
> directories. So put a check for filetype and return EBADF if
> lo_flush() is called on a non-regular file.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 8ba79f503a..48a109d3f6 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      struct lo_data *lo = lo_data(req);
>  
>      inode = lo_inode(req, ino);
> -    if (!inode) {
> +    if (!inode || !S_ISREG(inode->filetype)) {
>          fuse_reply_err(req, EBADF);

Does that need a lo_inode_put(lo, &inode) in the new case?

Dave

>          return;
>      }
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Currently lo_flush() is written in such a way that it expects to receive
> a FLUSH requests on a regular file (and not directories). For example,
> we call lo_fi_fd() which searches lo->fd_map. If we open directories
> using opendir(), we keep don't keep track of these in lo->fd_map instead
> we keep them in lo->dir_map. So we expect lo_flush() to be called on
> regular files only.
> 
> Even linux fuse client calls FLUSH only for regular files and not
> directories. So put a check for filetype and return EBADF if
> lo_flush() is called on a non-regular file.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 8ba79f503a..48a109d3f6 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      struct lo_data *lo = lo_data(req);
>  
>      inode = lo_inode(req, ino);
> -    if (!inode) {
> +    if (!inode || !S_ISREG(inode->filetype)) {
>          fuse_reply_err(req, EBADF);

Does that need a lo_inode_put(lo, &inode) in the new case?

Dave

>          return;
>      }
> -- 
> 2.25.4
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Vivek Goyal]

On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > Currently lo_flush() is written in such a way that it expects to receive
> > a FLUSH requests on a regular file (and not directories). For example,
> > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > regular files only.
> > 
> > Even linux fuse client calls FLUSH only for regular files and not
> > directories. So put a check for filetype and return EBADF if
> > lo_flush() is called on a non-regular file.
> > 
> > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 8ba79f503a..48a109d3f6 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> >      struct lo_data *lo = lo_data(req);
> >  
> >      inode = lo_inode(req, ino);
> > -    if (!inode) {
> > +    if (!inode || !S_ISREG(inode->filetype)) {
> >          fuse_reply_err(req, EBADF);
> 
> Does that need a lo_inode_put(lo, &inode) in the new case?

Good catch. Yes if inode is valid but file type is not regular, we need
to put inode reference.

Do you want me to post a new patch or you will like to take care of
it.

Vivek

Re: [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Vivek Goyal]

On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > Currently lo_flush() is written in such a way that it expects to receive
> > a FLUSH requests on a regular file (and not directories). For example,
> > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > regular files only.
> > 
> > Even linux fuse client calls FLUSH only for regular files and not
> > directories. So put a check for filetype and return EBADF if
> > lo_flush() is called on a non-regular file.
> > 
> > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  tools/virtiofsd/passthrough_ll.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 8ba79f503a..48a109d3f6 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> >      struct lo_data *lo = lo_data(req);
> >  
> >      inode = lo_inode(req, ino);
> > -    if (!inode) {
> > +    if (!inode || !S_ISREG(inode->filetype)) {
> >          fuse_reply_err(req, EBADF);
> 
> Does that need a lo_inode_put(lo, &inode) in the new case?

Good catch. Yes if inode is valid but file type is not regular, we need
to put inode reference.

Do you want me to post a new patch or you will like to take care of
it.

Vivek

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > Currently lo_flush() is written in such a way that it expects to receive
> > > a FLUSH requests on a regular file (and not directories). For example,
> > > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > > regular files only.
> > > 
> > > Even linux fuse client calls FLUSH only for regular files and not
> > > directories. So put a check for filetype and return EBADF if
> > > lo_flush() is called on a non-regular file.
> > > 
> > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 8ba79f503a..48a109d3f6 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > >      struct lo_data *lo = lo_data(req);
> > >  
> > >      inode = lo_inode(req, ino);
> > > -    if (!inode) {
> > > +    if (!inode || !S_ISREG(inode->filetype)) {
> > >          fuse_reply_err(req, EBADF);
> > 
> > Does that need a lo_inode_put(lo, &inode) in the new case?
> 
> Good catch. Yes if inode is valid but file type is not regular, we need
> to put inode reference.
> 
> Do you want me to post a new patch or you will like to take care of
> it.

OK, so if we make this :

  if (!inode) {
      fuse_reply_err(req, EBADF);
      return;
  }

  if (!S_ISREG(inode->filetype)) {
      lo_inode_put(lo_data(req), &inode);
      fuse_reply_err(req, EBADF);
      return;
  }

(Untested)

Dave

> 
> Vivek
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > Currently lo_flush() is written in such a way that it expects to receive
> > > a FLUSH requests on a regular file (and not directories). For example,
> > > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > > regular files only.
> > > 
> > > Even linux fuse client calls FLUSH only for regular files and not
> > > directories. So put a check for filetype and return EBADF if
> > > lo_flush() is called on a non-regular file.
> > > 
> > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 8ba79f503a..48a109d3f6 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > >      struct lo_data *lo = lo_data(req);
> > >  
> > >      inode = lo_inode(req, ino);
> > > -    if (!inode) {
> > > +    if (!inode || !S_ISREG(inode->filetype)) {
> > >          fuse_reply_err(req, EBADF);
> > 
> > Does that need a lo_inode_put(lo, &inode) in the new case?
> 
> Good catch. Yes if inode is valid but file type is not regular, we need
> to put inode reference.
> 
> Do you want me to post a new patch or you will like to take care of
> it.

OK, so if we make this :

  if (!inode) {
      fuse_reply_err(req, EBADF);
      return;
  }

  if (!S_ISREG(inode->filetype)) {
      lo_inode_put(lo_data(req), &inode);
      fuse_reply_err(req, EBADF);
      return;
  }

(Untested)

Dave

> 
> Vivek
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Vivek Goyal]

On Thu, Dec 10, 2020 at 08:14:31PM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > Currently lo_flush() is written in such a way that it expects to receive
> > > > a FLUSH requests on a regular file (and not directories). For example,
> > > > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > > > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > > > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > > > regular files only.
> > > > 
> > > > Even linux fuse client calls FLUSH only for regular files and not
> > > > directories. So put a check for filetype and return EBADF if
> > > > lo_flush() is called on a non-regular file.
> > > > 
> > > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > ---
> > > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index 8ba79f503a..48a109d3f6 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > >      struct lo_data *lo = lo_data(req);
> > > >  
> > > >      inode = lo_inode(req, ino);
> > > > -    if (!inode) {
> > > > +    if (!inode || !S_ISREG(inode->filetype)) {
> > > >          fuse_reply_err(req, EBADF);
> > > 
> > > Does that need a lo_inode_put(lo, &inode) in the new case?
> > 
> > Good catch. Yes if inode is valid but file type is not regular, we need
> > to put inode reference.
> > 
> > Do you want me to post a new patch or you will like to take care of
> > it.
> 
> OK, so if we make this :
> 
>   if (!inode) {
>       fuse_reply_err(req, EBADF);
>       return;
>   }
> 
>   if (!S_ISREG(inode->filetype)) {
>       lo_inode_put(lo_data(req), &inode);
>       fuse_reply_err(req, EBADF);
>       return;
>   }
> 
> (Untested)

Hi Dave,

Above looks good. For your convenience, I updated the patch and also
tested it by running blogbench and things look fine.

Vivek

Subject: virtiofsd: Check file type in lo_flush()

Currently lo_flush() is written in such a way that it expects to receive
a FLUSH requests on a regular file (and not directories). For example,
we call lo_fi_fd() which searches lo->fd_map. If we open directories
using opendir(), we keep don't keep track of these in lo->fd_map instead
we keep them in lo->dir_map. So we expect lo_flush() to be called on
regular files only.

Even linux fuse client calls FLUSH only for regular files and not
directories. So put a check for filetype and return EBADF if 
lo_flush() is called on a non-regular file.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:00:28.787669761 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:03:38.239496505 -0500
@@ -1973,6 +1973,12 @@ static void lo_flush(fuse_req_t req, fus
         return;
     }
 
+    if (!S_ISREG(inode->filetype)) {
+        lo_inode_put(lo, &inode);
+        fuse_reply_err(req, EBADF);
+        return;
+    }
+
     /* An fd is going away. Cleanup associated posix locks */
     if (lo->posix_lock) {
         pthread_mutex_lock(&inode->plock_mutex);

Re: [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Vivek Goyal]

On Thu, Dec 10, 2020 at 08:14:31PM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > Currently lo_flush() is written in such a way that it expects to receive
> > > > a FLUSH requests on a regular file (and not directories). For example,
> > > > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > > > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > > > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > > > regular files only.
> > > > 
> > > > Even linux fuse client calls FLUSH only for regular files and not
> > > > directories. So put a check for filetype and return EBADF if
> > > > lo_flush() is called on a non-regular file.
> > > > 
> > > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > ---
> > > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index 8ba79f503a..48a109d3f6 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > >      struct lo_data *lo = lo_data(req);
> > > >  
> > > >      inode = lo_inode(req, ino);
> > > > -    if (!inode) {
> > > > +    if (!inode || !S_ISREG(inode->filetype)) {
> > > >          fuse_reply_err(req, EBADF);
> > > 
> > > Does that need a lo_inode_put(lo, &inode) in the new case?
> > 
> > Good catch. Yes if inode is valid but file type is not regular, we need
> > to put inode reference.
> > 
> > Do you want me to post a new patch or you will like to take care of
> > it.
> 
> OK, so if we make this :
> 
>   if (!inode) {
>       fuse_reply_err(req, EBADF);
>       return;
>   }
> 
>   if (!S_ISREG(inode->filetype)) {
>       lo_inode_put(lo_data(req), &inode);
>       fuse_reply_err(req, EBADF);
>       return;
>   }
> 
> (Untested)

Hi Dave,

Above looks good. For your convenience, I updated the patch and also
tested it by running blogbench and things look fine.

Vivek

Subject: virtiofsd: Check file type in lo_flush()

Currently lo_flush() is written in such a way that it expects to receive
a FLUSH requests on a regular file (and not directories). For example,
we call lo_fi_fd() which searches lo->fd_map. If we open directories
using opendir(), we keep don't keep track of these in lo->fd_map instead
we keep them in lo->dir_map. So we expect lo_flush() to be called on
regular files only.

Even linux fuse client calls FLUSH only for regular files and not
directories. So put a check for filetype and return EBADF if 
lo_flush() is called on a non-regular file.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 tools/virtiofsd/passthrough_ll.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===================================================================
--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:00:28.787669761 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:03:38.239496505 -0500
@@ -1973,6 +1973,12 @@ static void lo_flush(fuse_req_t req, fus
         return;
     }
 
+    if (!S_ISREG(inode->filetype)) {
+        lo_inode_put(lo, &inode);
+        fuse_reply_err(req, EBADF);
+        return;
+    }
+
     /* An fd is going away. Cleanup associated posix locks */
     if (lo->posix_lock) {
         pthread_mutex_lock(&inode->plock_mutex);

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Dec 10, 2020 at 08:14:31PM +0000, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> > > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > > Currently lo_flush() is written in such a way that it expects to receive
> > > > > a FLUSH requests on a regular file (and not directories). For example,
> > > > > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > > > > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > > > > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > > > > regular files only.
> > > > > 
> > > > > Even linux fuse client calls FLUSH only for regular files and not
> > > > > directories. So put a check for filetype and return EBADF if
> > > > > lo_flush() is called on a non-regular file.
> > > > > 
> > > > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > > ---
> > > > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > > index 8ba79f503a..48a109d3f6 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > > >      struct lo_data *lo = lo_data(req);
> > > > >  
> > > > >      inode = lo_inode(req, ino);
> > > > > -    if (!inode) {
> > > > > +    if (!inode || !S_ISREG(inode->filetype)) {
> > > > >          fuse_reply_err(req, EBADF);
> > > > 
> > > > Does that need a lo_inode_put(lo, &inode) in the new case?
> > > 
> > > Good catch. Yes if inode is valid but file type is not regular, we need
> > > to put inode reference.
> > > 
> > > Do you want me to post a new patch or you will like to take care of
> > > it.
> > 
> > OK, so if we make this :
> > 
> >   if (!inode) {
> >       fuse_reply_err(req, EBADF);
> >       return;
> >   }
> > 
> >   if (!S_ISREG(inode->filetype)) {
> >       lo_inode_put(lo_data(req), &inode);
> >       fuse_reply_err(req, EBADF);
> >       return;
> >   }
> > 
> > (Untested)
> 
> Hi Dave,
> 
> Above looks good. For your convenience, I updated the patch and also
> tested it by running blogbench and things look fine.
> 
> Vivek
> 
> Subject: virtiofsd: Check file type in lo_flush()
> 
> Currently lo_flush() is written in such a way that it expects to receive
> a FLUSH requests on a regular file (and not directories). For example,
> we call lo_fi_fd() which searches lo->fd_map. If we open directories
> using opendir(), we keep don't keep track of these in lo->fd_map instead
> we keep them in lo->dir_map. So we expect lo_flush() to be called on
> regular files only.
> 
> Even linux fuse client calls FLUSH only for regular files and not
> directories. So put a check for filetype and return EBADF if 
> lo_flush() is called on a non-regular file.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

OK< thanks

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and queued.
(I've dropped the Tested-by since it's had a bit of forceful rework).

> ---
>  tools/virtiofsd/passthrough_ll.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:00:28.787669761 -0500
> +++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:03:38.239496505 -0500
> @@ -1973,6 +1973,12 @@ static void lo_flush(fuse_req_t req, fus
>          return;
>      }
>  
> +    if (!S_ISREG(inode->filetype)) {
> +        lo_inode_put(lo, &inode);
> +        fuse_reply_err(req, EBADF);
> +        return;
> +    }
> +
>      /* An fd is going away. Cleanup associated posix locks */
>      if (lo->posix_lock) {
>          pthread_mutex_lock(&inode->plock_mutex);
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/3] virtiofsd: Check file type in lo_flush()

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Dec 10, 2020 at 08:14:31PM +0000, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > On Thu, Dec 10, 2020 at 08:03:03PM +0000, Dr. David Alan Gilbert wrote:
> > > > * Vivek Goyal (vgoyal@redhat.com) wrote:
> > > > > Currently lo_flush() is written in such a way that it expects to receive
> > > > > a FLUSH requests on a regular file (and not directories). For example,
> > > > > we call lo_fi_fd() which searches lo->fd_map. If we open directories
> > > > > using opendir(), we keep don't keep track of these in lo->fd_map instead
> > > > > we keep them in lo->dir_map. So we expect lo_flush() to be called on
> > > > > regular files only.
> > > > > 
> > > > > Even linux fuse client calls FLUSH only for regular files and not
> > > > > directories. So put a check for filetype and return EBADF if
> > > > > lo_flush() is called on a non-regular file.
> > > > > 
> > > > > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > > ---
> > > > >  tools/virtiofsd/passthrough_ll.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > > index 8ba79f503a..48a109d3f6 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -1968,7 +1968,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > > >      struct lo_data *lo = lo_data(req);
> > > > >  
> > > > >      inode = lo_inode(req, ino);
> > > > > -    if (!inode) {
> > > > > +    if (!inode || !S_ISREG(inode->filetype)) {
> > > > >          fuse_reply_err(req, EBADF);
> > > > 
> > > > Does that need a lo_inode_put(lo, &inode) in the new case?
> > > 
> > > Good catch. Yes if inode is valid but file type is not regular, we need
> > > to put inode reference.
> > > 
> > > Do you want me to post a new patch or you will like to take care of
> > > it.
> > 
> > OK, so if we make this :
> > 
> >   if (!inode) {
> >       fuse_reply_err(req, EBADF);
> >       return;
> >   }
> > 
> >   if (!S_ISREG(inode->filetype)) {
> >       lo_inode_put(lo_data(req), &inode);
> >       fuse_reply_err(req, EBADF);
> >       return;
> >   }
> > 
> > (Untested)
> 
> Hi Dave,
> 
> Above looks good. For your convenience, I updated the patch and also
> tested it by running blogbench and things look fine.
> 
> Vivek
> 
> Subject: virtiofsd: Check file type in lo_flush()
> 
> Currently lo_flush() is written in such a way that it expects to receive
> a FLUSH requests on a regular file (and not directories). For example,
> we call lo_fi_fd() which searches lo->fd_map. If we open directories
> using opendir(), we keep don't keep track of these in lo->fd_map instead
> we keep them in lo->dir_map. So we expect lo_flush() to be called on
> regular files only.
> 
> Even linux fuse client calls FLUSH only for regular files and not
> directories. So put a check for filetype and return EBADF if 
> lo_flush() is called on a non-regular file.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

OK< thanks

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and queued.
(I've dropped the Tested-by since it's had a bit of forceful rework).

> ---
>  tools/virtiofsd/passthrough_ll.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
> ===================================================================
> --- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:00:28.787669761 -0500
> +++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2020-12-11 09:03:38.239496505 -0500
> @@ -1973,6 +1973,12 @@ static void lo_flush(fuse_req_t req, fus
>          return;
>      }
>  
> +    if (!S_ISREG(inode->filetype)) {
> +        lo_inode_put(lo, &inode);
> +        fuse_reply_err(req, EBADF);
> +        return;
> +    }
> +
>      /* An fd is going away. Cleanup associated posix locks */
>      if (lo->posix_lock) {
>          pthread_mutex_lock(&inode->plock_mutex);
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Virtio-fs] ceph + freeipa ubuntu/fedora common small bug

[Harry G. Coin]

FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
with ceph.  Identical bitmask report.

...

Fixing /var/run/ceph ownership....done

Cannot set file attribute for '/var/log/journal', value=0x00800000,
mask=0x00800000, ignoring: Function not implemented

Cannot set file attribute for
'/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000,
mask=0x00800000, ignoring: Function not implemented

...

Host has xattrs on, btrfs.

ceph + freeipa ubuntu/fedora common small bug

[Harry G. Coin]

FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
with ceph.  Identical bitmask report.

...

Fixing /var/run/ceph ownership....done

Cannot set file attribute for '/var/log/journal', value=0x00800000,
mask=0x00800000, ignoring: Function not implemented

Cannot set file attribute for
'/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000,
mask=0x00800000, ignoring: Function not implemented

...

Host has xattrs on, btrfs.

Re: [Virtio-fs] ceph + freeipa ubuntu/fedora common small bug

[Dr. David Alan Gilbert]

* Harry G. Coin (hgcoin@gmail.com) wrote:
> FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
> with ceph.  Identical bitmask report.
> 
> ...
> 
> Fixing /var/run/ceph ownership....done
> 
> Cannot set file attribute for '/var/log/journal', value=0x00800000,
> mask=0x00800000, ignoring: Function not implemented
> 
> Cannot set file attribute for
> '/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000,
> mask=0x00800000, ignoring: Function not implemented

This looks like it comes out of systemd's  src/tmpfiles/tmpfiles.c:

        r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL);
        if (r < 0)
                log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : LOG_WARNING,
                               r,
                               "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, ignoring: %m",
                               path, item->attribute_value, item->attribute_mask);

and it's chattr_fd is in it's src/basic/chattr-util.c
which is using FS_IOC_GET/SETFLAGS, which seems to be an older
way of doing things.

Now, is that supposed to promote itself to a newer call or is it OK?

Dave

> ...
> 
> Host has xattrs on, btrfs.
> 
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: ceph + freeipa ubuntu/fedora common small bug

[Dr. David Alan Gilbert]

* Harry G. Coin (hgcoin@gmail.com) wrote:
> FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
> with ceph.  Identical bitmask report.
> 
> ...
> 
> Fixing /var/run/ceph ownership....done
> 
> Cannot set file attribute for '/var/log/journal', value=0x00800000,
> mask=0x00800000, ignoring: Function not implemented
> 
> Cannot set file attribute for
> '/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000,
> mask=0x00800000, ignoring: Function not implemented

This looks like it comes out of systemd's  src/tmpfiles/tmpfiles.c:

        r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL);
        if (r < 0)
                log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : LOG_WARNING,
                               r,
                               "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, ignoring: %m",
                               path, item->attribute_value, item->attribute_mask);

and it's chattr_fd is in it's src/basic/chattr-util.c
which is using FS_IOC_GET/SETFLAGS, which seems to be an older
way of doing things.

Now, is that supposed to promote itself to a newer call or is it OK?

Dave

> ...
> 
> Host has xattrs on, btrfs.
> 
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] ceph + freeipa ubuntu/fedora common small bug

[Vivek Goyal]

On Fri, Dec 11, 2020 at 11:05:22AM +0000, Dr. David Alan Gilbert wrote:
> * Harry G. Coin (hgcoin@gmail.com) wrote:
> > FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
> > with ceph.  Identical bitmask report.
> > 
> > ...
> > 
> > Fixing /var/run/ceph ownership....done
> > 
> > Cannot set file attribute for '/var/log/journal', value=0x00800000,
> > mask=0x00800000, ignoring: Function not implemented
> > 
> > Cannot set file attribute for
> > '/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000,
> > mask=0x00800000, ignoring: Function not implemented
> 
> This looks like it comes out of systemd's  src/tmpfiles/tmpfiles.c:
> 
>         r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL);
>         if (r < 0)
>                 log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : LOG_WARNING,
>                                r,
>                                "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, ignoring: %m",
>                                path, item->attribute_value, item->attribute_mask);
> 
> and it's chattr_fd is in it's src/basic/chattr-util.c
> which is using FS_IOC_GET/SETFLAGS, which seems to be an older
> way of doing things.
> 
> Now, is that supposed to promote itself to a newer call or is it OK?

I see that we don't have any ->ioctl function registered in
passthrough_ll.c and that's why do_ioctl() (fuse_lowlevel.c) will
return -ENOSYS.

So we probably need to modify passthrough_ll.c to support some select
ioctls. Right now it looks like all fs ioctls will return -ENOSYS.

I tried "chattr +i foo.txt" and that return -ENOSYS as well.

Vivek

Re: ceph + freeipa ubuntu/fedora common small bug

[Vivek Goyal]

On Fri, Dec 11, 2020 at 11:05:22AM +0000, Dr. David Alan Gilbert wrote:
> * Harry G. Coin (hgcoin@gmail.com) wrote:
> > FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
> > with ceph.  Identical bitmask report.
> > 
> > ...
> > 
> > Fixing /var/run/ceph ownership....done
> > 
> > Cannot set file attribute for '/var/log/journal', value=0x00800000,
> > mask=0x00800000, ignoring: Function not implemented
> > 
> > Cannot set file attribute for
> > '/var/log/journal/fd007229322043ad8778c214d19ed3ac', value=0x00800000,
> > mask=0x00800000, ignoring: Function not implemented
> 
> This looks like it comes out of systemd's  src/tmpfiles/tmpfiles.c:
> 
>         r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL);
>         if (r < 0)
>                 log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG : LOG_WARNING,
>                                r,
>                                "Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x, ignoring: %m",
>                                path, item->attribute_value, item->attribute_mask);
> 
> and it's chattr_fd is in it's src/basic/chattr-util.c
> which is using FS_IOC_GET/SETFLAGS, which seems to be an older
> way of doing things.
> 
> Now, is that supposed to promote itself to a newer call or is it OK?

I see that we don't have any ->ioctl function registered in
passthrough_ll.c and that's why do_ioctl() (fuse_lowlevel.c) will
return -ENOSYS.

So we probably need to modify passthrough_ll.c to support some select
ioctls. Right now it looks like all fs ioctls will return -ENOSYS.

I tried "chattr +i foo.txt" and that return -ENOSYS as well.

Vivek

Re: [Virtio-fs] ceph + freeipa ubuntu/fedora common small bug

[Harry Coin]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

It's the latest ubuntu and fedora distros.


On December 11, 2020 5:05:22 AM CST, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>* Harry G. Coin (hgcoin@gmail.com) wrote:
>> FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
>> with ceph.  Identical bitmask report.
>> 
>> ...
>> 
>> Fixing /var/run/ceph ownership....done
>> 
>> Cannot set file attribute for '/var/log/journal', value=0x00800000,
>> mask=0x00800000, ignoring: Function not implemented
>> 
>> Cannot set file attribute for
>> '/var/log/journal/fd007229322043ad8778c214d19ed3ac',
>value=0x00800000,
>> mask=0x00800000, ignoring: Function not implemented
>
>This looks like it comes out of systemd's  src/tmpfiles/tmpfiles.c:
>
>        r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL);
>        if (r < 0)
>log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG :
>LOG_WARNING,
>                               r,
>"Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x,
>ignoring: %m",
>                    path, item->attribute_value, item->attribute_mask);
>
>and it's chattr_fd is in it's src/basic/chattr-util.c
>which is using FS_IOC_GET/SETFLAGS, which seems to be an older
>way of doing things.
>
>Now, is that supposed to promote itself to a newer call or is it OK?
>
>Dave
>
>> ...
>> 
>> Host has xattrs on, btrfs.
>> 
>> 
>> 
>-- 
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[-- Attachment #2: Type: text/html, Size: 2138 bytes --]

Re: ceph + freeipa ubuntu/fedora common small bug

[Harry Coin]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

It's the latest ubuntu and fedora distros.


On December 11, 2020 5:05:22 AM CST, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>* Harry G. Coin (hgcoin@gmail.com) wrote:
>> FYI.  Same thing we saw on Fedora installing freeipa, this on ubuntu
>> with ceph.  Identical bitmask report.
>> 
>> ...
>> 
>> Fixing /var/run/ceph ownership....done
>> 
>> Cannot set file attribute for '/var/log/journal', value=0x00800000,
>> mask=0x00800000, ignoring: Function not implemented
>> 
>> Cannot set file attribute for
>> '/var/log/journal/fd007229322043ad8778c214d19ed3ac',
>value=0x00800000,
>> mask=0x00800000, ignoring: Function not implemented
>
>This looks like it comes out of systemd's  src/tmpfiles/tmpfiles.c:
>
>        r = chattr_fd(procfs_fd, f, item->attribute_mask, NULL);
>        if (r < 0)
>log_full_errno(IN_SET(r, -ENOTTY, -EOPNOTSUPP) ? LOG_DEBUG :
>LOG_WARNING,
>                               r,
>"Cannot set file attribute for '%s', value=0x%08x, mask=0x%08x,
>ignoring: %m",
>                    path, item->attribute_value, item->attribute_mask);
>
>and it's chattr_fd is in it's src/basic/chattr-util.c
>which is using FS_IOC_GET/SETFLAGS, which seems to be an older
>way of doing things.
>
>Now, is that supposed to promote itself to a newer call or is it OK?
>
>Dave
>
>> ...
>> 
>> Host has xattrs on, btrfs.
>> 
>> 
>> 
>-- 
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[-- Attachment #2: Type: text/html, Size: 2138 bytes --]

Re: [Virtio-fs] [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201207183021.22752-1-vgoyal@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201207183021.22752-1-vgoyal@redhat.com
Subject: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20201207183021.22752-1-vgoyal@redhat.com -> patchew/20201207183021.22752-1-vgoyal@redhat.com
Switched to a new branch 'test'
86f3bb1 virtiofsd: Check file type in lo_flush()
92a5c9d virtiofsd: Disable posix_lock hash table if remote locks are not enabled
fbdcaf1 virtiofsd: Set up posix_lock hash table for root inode

=== OUTPUT BEGIN ===
1/3 Checking commit fbdcaf172aed (virtiofsd: Set up posix_lock hash table for root inode)
ERROR: suspect code indent for conditional statements (4, 7)
#40: FILE: tools/virtiofsd/passthrough_ll.c:3401:
+    if (lo->root.posix_locks)
+       g_hash_table_destroy(lo->root.posix_locks);

ERROR: braces {} are necessary for all arms of this statement
#40: FILE: tools/virtiofsd/passthrough_ll.c:3401:
+    if (lo->root.posix_locks)
[...]

total: 2 errors, 0 warnings, 19 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit 92a5c9d35c7d (virtiofsd: Disable posix_lock hash table if remote locks are not enabled)
3/3 Checking commit 86f3bb1cf5b8 (virtiofsd: Check file type in lo_flush())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201207183021.22752-1-vgoyal@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201207183021.22752-1-vgoyal@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201207183021.22752-1-vgoyal@redhat.com
Subject: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20201207183021.22752-1-vgoyal@redhat.com -> patchew/20201207183021.22752-1-vgoyal@redhat.com
Switched to a new branch 'test'
86f3bb1 virtiofsd: Check file type in lo_flush()
92a5c9d virtiofsd: Disable posix_lock hash table if remote locks are not enabled
fbdcaf1 virtiofsd: Set up posix_lock hash table for root inode

=== OUTPUT BEGIN ===
1/3 Checking commit fbdcaf172aed (virtiofsd: Set up posix_lock hash table for root inode)
ERROR: suspect code indent for conditional statements (4, 7)
#40: FILE: tools/virtiofsd/passthrough_ll.c:3401:
+    if (lo->root.posix_locks)
+       g_hash_table_destroy(lo->root.posix_locks);

ERROR: braces {} are necessary for all arms of this statement
#40: FILE: tools/virtiofsd/passthrough_ll.c:3401:
+    if (lo->root.posix_locks)
[...]

total: 2 errors, 0 warnings, 19 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit 92a5c9d35c7d (virtiofsd: Disable posix_lock hash table if remote locks are not enabled)
3/3 Checking commit 86f3bb1cf5b8 (virtiofsd: Check file type in lo_flush())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201207183021.22752-1-vgoyal@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Virtio-fs] [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Laszlo Ersek]

Hi Vivek,

On 12/07/20 19:30, Vivek Goyal wrote:
> Laszlo is writing a virtiofs client for OVMF and noticed that if he
> sends fuse FLUSH command for directory object, virtiofsd crashes.
> virtiofsd does not expect a FLUSH arriving for a directory object.
> 
> This patch series has one of the patches which fixes that. It also
> has couple of posix lock fixes as a result of lo_flush() related debugging.
> 
> Vivek Goyal (3):
>   virtiofsd: Set up posix_lock hash table for root inode
>   virtiofsd: Disable posix_lock hash table if remote locks are not
>     enabled
>   virtiofsd: Check file type in lo_flush()
> 
>  tools/virtiofsd/passthrough_ll.c | 54 +++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 15 deletions(-)
> 

I put back the (wrong) FLUSH for the root dir into my code temporarily, to reproduce the crash (it does, with v5.2.0-rc4).

Then I applied your series [*], and retested.

[*] I'm unsure about the email you sent in response to 1/3, namely <https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01504.html>; I ignored that when applying the patches.

Indeed now I get a graceful -EBADF:

[13316825985314] [ID: 00000004] unique: 60, opcode: FLUSH (25), nodeid: 1, insize: 64, pid: 1
[13316825993517] [ID: 00000004]    unique: 60, error: -9 (Bad file descriptor), outsize: 16

For whichever patch in the series my testing is relevant:

Tested-by: Laszlo Ersek <lersek@redhat.com>

(I'm having some difficulty figuring out which patch(es) should carry my T-b.

- I think I didn't really test patch#2 with the above, so that one should likely not get the T-b

- I think patch#3 is what I really tested.

- But, if that's the case, doesn't patch#3 make the fix in patch#1 untestable, in my scenario? I believe the code is no longer reached in lo_flush(), due to patch#3, where the change from patch#1 would matter. Patch#1 seems correct, and the last paragraph of its commit message relevant, but I think my testing currently only covered patch#3.

I'll let you decide where to apply my T-b.)

Thanks!
Laszlo

Re: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Laszlo Ersek]

Hi Vivek,

On 12/07/20 19:30, Vivek Goyal wrote:
> Laszlo is writing a virtiofs client for OVMF and noticed that if he
> sends fuse FLUSH command for directory object, virtiofsd crashes.
> virtiofsd does not expect a FLUSH arriving for a directory object.
> 
> This patch series has one of the patches which fixes that. It also
> has couple of posix lock fixes as a result of lo_flush() related debugging.
> 
> Vivek Goyal (3):
>   virtiofsd: Set up posix_lock hash table for root inode
>   virtiofsd: Disable posix_lock hash table if remote locks are not
>     enabled
>   virtiofsd: Check file type in lo_flush()
> 
>  tools/virtiofsd/passthrough_ll.c | 54 +++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 15 deletions(-)
> 

I put back the (wrong) FLUSH for the root dir into my code temporarily, to reproduce the crash (it does, with v5.2.0-rc4).

Then I applied your series [*], and retested.

[*] I'm unsure about the email you sent in response to 1/3, namely <https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01504.html>; I ignored that when applying the patches.

Indeed now I get a graceful -EBADF:

[13316825985314] [ID: 00000004] unique: 60, opcode: FLUSH (25), nodeid: 1, insize: 64, pid: 1
[13316825993517] [ID: 00000004]    unique: 60, error: -9 (Bad file descriptor), outsize: 16

For whichever patch in the series my testing is relevant:

Tested-by: Laszlo Ersek <lersek@redhat.com>

(I'm having some difficulty figuring out which patch(es) should carry my T-b.

- I think I didn't really test patch#2 with the above, so that one should likely not get the T-b

- I think patch#3 is what I really tested.

- But, if that's the case, doesn't patch#3 make the fix in patch#1 untestable, in my scenario? I believe the code is no longer reached in lo_flush(), due to patch#3, where the change from patch#1 would matter. Patch#1 seems correct, and the last paragraph of its commit message relevant, but I think my testing currently only covered patch#3.

I'll let you decide where to apply my T-b.)

Thanks!
Laszlo

Re: [Virtio-fs] [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Vivek Goyal]

On Tue, Dec 08, 2020 at 05:51:34AM +0100, Laszlo Ersek wrote:
> Hi Vivek,
> 
> On 12/07/20 19:30, Vivek Goyal wrote:
> > Laszlo is writing a virtiofs client for OVMF and noticed that if he
> > sends fuse FLUSH command for directory object, virtiofsd crashes.
> > virtiofsd does not expect a FLUSH arriving for a directory object.
> > 
> > This patch series has one of the patches which fixes that. It also
> > has couple of posix lock fixes as a result of lo_flush() related debugging.
> > 
> > Vivek Goyal (3):
> >   virtiofsd: Set up posix_lock hash table for root inode
> >   virtiofsd: Disable posix_lock hash table if remote locks are not
> >     enabled
> >   virtiofsd: Check file type in lo_flush()
> > 
> >  tools/virtiofsd/passthrough_ll.c | 54 +++++++++++++++++++++++---------
> >  1 file changed, 39 insertions(+), 15 deletions(-)
> > 
> 
> I put back the (wrong) FLUSH for the root dir into my code temporarily, to reproduce the crash (it does, with v5.2.0-rc4).
> 
> Then I applied your series [*], and retested.
> 
> [*] I'm unsure about the email you sent in response to 1/3, namely <https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01504.html>; I ignored that when applying the patches.

Hi Laszlo,

Thank you for the testing.

I reposed patch 1 to take care of coding style issues. Functionally both
the versions are same.

> 
> Indeed now I get a graceful -EBADF:
> 
> [13316825985314] [ID: 00000004] unique: 60, opcode: FLUSH (25), nodeid: 1, insize: 64, pid: 1
> [13316825993517] [ID: 00000004]    unique: 60, error: -9 (Bad file descriptor), outsize: 16
> 
> For whichever patch in the series my testing is relevant:
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> (I'm having some difficulty figuring out which patch(es) should carry my T-b.
> 
> - I think I didn't really test patch#2 with the above, so that one should likely not get the T-b
> 
> - I think patch#3 is what I really tested.
> 
> - But, if that's the case, doesn't patch#3 make the fix in patch#1 untestable, in my scenario? I believe the code is no longer reached in lo_flush(), due to patch#3, where the change from patch#1 would matter. Patch#1 seems correct, and the last paragraph of its commit message relevant, but I think my testing currently only covered patch#3.
> 
> I'll let you decide where to apply my T-b.)

David Gilbert can add your Tested-by: while applying this patch series.
I think adding it to patch 3 makes most sense.

Thanks
Vivek

Re: [PATCH 0/3] virtiofsd: Fix lo_flush() and inode->posix_lock init

[Vivek Goyal]

On Tue, Dec 08, 2020 at 05:51:34AM +0100, Laszlo Ersek wrote:
> Hi Vivek,
> 
> On 12/07/20 19:30, Vivek Goyal wrote:
> > Laszlo is writing a virtiofs client for OVMF and noticed that if he
> > sends fuse FLUSH command for directory object, virtiofsd crashes.
> > virtiofsd does not expect a FLUSH arriving for a directory object.
> > 
> > This patch series has one of the patches which fixes that. It also
> > has couple of posix lock fixes as a result of lo_flush() related debugging.
> > 
> > Vivek Goyal (3):
> >   virtiofsd: Set up posix_lock hash table for root inode
> >   virtiofsd: Disable posix_lock hash table if remote locks are not
> >     enabled
> >   virtiofsd: Check file type in lo_flush()
> > 
> >  tools/virtiofsd/passthrough_ll.c | 54 +++++++++++++++++++++++---------
> >  1 file changed, 39 insertions(+), 15 deletions(-)
> > 
> 
> I put back the (wrong) FLUSH for the root dir into my code temporarily, to reproduce the crash (it does, with v5.2.0-rc4).
> 
> Then I applied your series [*], and retested.
> 
> [*] I'm unsure about the email you sent in response to 1/3, namely <https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01504.html>; I ignored that when applying the patches.

Hi Laszlo,

Thank you for the testing.

I reposed patch 1 to take care of coding style issues. Functionally both
the versions are same.

> 
> Indeed now I get a graceful -EBADF:
> 
> [13316825985314] [ID: 00000004] unique: 60, opcode: FLUSH (25), nodeid: 1, insize: 64, pid: 1
> [13316825993517] [ID: 00000004]    unique: 60, error: -9 (Bad file descriptor), outsize: 16
> 
> For whichever patch in the series my testing is relevant:
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> (I'm having some difficulty figuring out which patch(es) should carry my T-b.
> 
> - I think I didn't really test patch#2 with the above, so that one should likely not get the T-b
> 
> - I think patch#3 is what I really tested.
> 
> - But, if that's the case, doesn't patch#3 make the fix in patch#1 untestable, in my scenario? I believe the code is no longer reached in lo_flush(), due to patch#3, where the change from patch#1 would matter. Patch#1 seems correct, and the last paragraph of its commit message relevant, but I think my testing currently only covered patch#3.
> 
> I'll let you decide where to apply my T-b.)

David Gilbert can add your Tested-by: while applying this patch series.
I think adding it to patch 3 makes most sense.

Thanks
Vivek