This was a single patch until v3. Some preliminary cleanups were
introduced for submounts in this v4.
This can be tested with a custom virtiofsd implementing FUSE_SYNCFS, here:
https://gitlab.com/gkurz/qemu/-/tree/fuse-sync
v4: - submount fixes
- set nodeid of the superblock in the request (Miklos)
v3: - just keep a 64-bit padding field in the arg struct (Vivek)
v2: - clarify compatibility with older servers in changelog (Vivek)
- ignore the wait == 0 case (Miklos)
- 64-bit aligned argument structure (Vivek, Miklos)
Greg Kurz (5):
fuse: Fix leak in fuse_dentry_automount() error path
fuse: Call vfs_get_tree() for submounts
fuse: Make fuse_fill_super_submount() static
virtiofs: Skip submounts in sget_fc()
virtiofs: propagate sync() to file server
fs/fuse/dir.c | 45 +++++---------------
fs/fuse/fuse_i.h | 12 +++---
fs/fuse/inode.c | 87 ++++++++++++++++++++++++++++++++++++++-
fs/fuse/virtio_fs.c | 9 ++++
include/uapi/linux/fuse.h | 10 ++++-
5 files changed, 120 insertions(+), 43 deletions(-)
--
2.26.3
Greg Kurz
2021-May-20 15:46 UTC
[PATCH v4 1/5] fuse: Fix leak in fuse_dentry_automount() error path
Some rollback was forgotten during the addition of crossmounts.
Fixes: bf109c64040f ("fuse: implement crossmounts")
Cc: mreitz at redhat.com
Signed-off-by: Greg Kurz <groug at kaod.org>
---
fs/fuse/dir.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 1b6c001a7dd1..fb2af70596c3 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -339,8 +339,11 @@ static struct vfsmount *fuse_dentry_automount(struct path
*path)
/* Initialize superblock, making @mp_fi its root */
err = fuse_fill_super_submount(sb, mp_fi);
- if (err)
+ if (err) {
+ fuse_conn_put(fc);
+ kfree(fm);
goto out_put_sb;
+ }
sb->s_flags |= SB_ACTIVE;
fsc->root = dget(sb->s_root);
--
2.26.3
We don't set the SB_BORN flag on submounts superblocks. This is wrong
as these superblocks are then considered as partially constructed or
dying in the rest of the code and can break some assumptions.
One such case is when you have a virtiofs filesystem and you try to
mount it again : virtio_fs_get_tree() tries to obtain a superblock
with sget_fc(). The matching criteria in virtio_fs_test_super() is
the pointer of the underlying virtiofs device, which is shared by
the root mount and its submounts. This means that any submount can
be picked up instead of the root mount. This is itself a bug :
submounts should be ignored in this case. But, most importantly, it
then triggers an infinite loop in sget_fc() because it fails to grab
the superblock (very easy to reproduce).
The only viable solution is to set SB_BORN at some point. This
must be done with vfs_get_tree() because setting SB_BORN requires
special care, i.e. a memory barrier for super_cache_count() which
can check SB_BORN without taking any lock.
This requires to split out some code from fuse_dentry_automount() to
a new dedicated fuse_get_tree_submount().
The fs_private field of the filesystem context isn't used with
submounts : hijack it to pass the FUSE inode of the mount point
down to fuse_get_tree_submount().
Finally, adapt virtiofs to use this.
Signed-off-by: Greg Kurz <groug at kaod.org>
---
fs/fuse/dir.c | 48 +++++++++++----------------------------------
fs/fuse/fuse_i.h | 6 ++++++
fs/fuse/inode.c | 43 ++++++++++++++++++++++++++++++++++++++++
fs/fuse/virtio_fs.c | 3 +++
4 files changed, 63 insertions(+), 37 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index fb2af70596c3..4c8dafe4f69e 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -309,12 +309,9 @@ static int fuse_dentry_delete(const struct dentry *dentry)
static struct vfsmount *fuse_dentry_automount(struct path *path)
{
struct fs_context *fsc;
- struct fuse_mount *parent_fm = get_fuse_mount_super(path->mnt->mnt_sb);
- struct fuse_conn *fc = parent_fm->fc;
struct fuse_mount *fm;
struct vfsmount *mnt;
struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
- struct super_block *sb;
int err;
fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type,
path->dentry);
@@ -323,36 +320,19 @@ static struct vfsmount *fuse_dentry_automount(struct path
*path)
goto out;
}
- err = -ENOMEM;
- fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
- if (!fm)
+ /*
+ * Hijack fsc->fs_private to pass the mount point inode to
+ * fuse_get_tree_submount(). It *must* be NULLified afterwards
+ * to avoid the inode pointer to be passed to kfree() when
+ * the context gets freed.
+ */
+ fsc->fs_private = mp_fi;
+ err = vfs_get_tree(fsc);
+ fsc->fs_private = NULL;
+ if (err)
goto out_put_fsc;
- fsc->s_fs_info = fm;
- sb = sget_fc(fsc, NULL, set_anon_super_fc);
- if (IS_ERR(sb)) {
- err = PTR_ERR(sb);
- kfree(fm);
- goto out_put_fsc;
- }
- fm->fc = fuse_conn_get(fc);
-
- /* Initialize superblock, making @mp_fi its root */
- err = fuse_fill_super_submount(sb, mp_fi);
- if (err) {
- fuse_conn_put(fc);
- kfree(fm);
- goto out_put_sb;
- }
-
- sb->s_flags |= SB_ACTIVE;
- fsc->root = dget(sb->s_root);
- /* We are done configuring the superblock, so unlock it */
- up_write(&sb->s_umount);
-
- down_write(&fc->killsb);
- list_add_tail(&fm->fc_entry, &fc->mounts);
- up_write(&fc->killsb);
+ fm = get_fuse_mount_super(fsc->root->d_sb);
/* Create the submount */
mnt = vfs_create_mount(fsc);
@@ -364,12 +344,6 @@ static struct vfsmount *fuse_dentry_automount(struct path
*path)
put_fs_context(fsc);
return mnt;
-out_put_sb:
- /*
- * Only jump here when fsc->root is NULL and sb is still locked
- * (otherwise put_fs_context() will put the superblock)
- */
- deactivate_locked_super(sb);
out_put_fsc:
put_fs_context(fsc);
out:
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 7e463e220053..d7fcf59a6a0e 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1090,6 +1090,12 @@ int fuse_fill_super_common(struct super_block *sb, struct
fuse_fs_context *ctx);
int fuse_fill_super_submount(struct super_block *sb,
struct fuse_inode *parent_fi);
+/*
+ * Get the mountable root for the submount
+ * @fsc: superblock configuration context
+ */
+int fuse_get_tree_submount(struct fs_context *fsc);
+
/*
* Remove the mount from the connection
*
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 393e36b74dc4..74e5205f203c 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1313,6 +1313,49 @@ int fuse_fill_super_submount(struct super_block *sb,
return 0;
}
+/* Filesystem context private data holds the FUSE inode of the mount point */
+int fuse_get_tree_submount(struct fs_context *fsc)
+{
+ struct fuse_mount *fm;
+ struct fuse_inode *mp_fi = fsc->fs_private;
+ struct fuse_conn *fc = get_fuse_conn(&mp_fi->inode);
+ struct super_block *sb;
+ int err;
+
+ fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
+ if (!fm)
+ return -ENOMEM;
+
+ fsc->s_fs_info = fm;
+ sb = sget_fc(fsc, NULL, set_anon_super_fc);
+ if (IS_ERR(sb)) {
+ kfree(fm);
+ return PTR_ERR(sb);
+ }
+ fm->fc = fuse_conn_get(fc);
+
+ /* Initialize superblock, making @mp_fi its root */
+ err = fuse_fill_super_submount(sb, mp_fi);
+ if (err) {
+ fuse_conn_put(fc);
+ deactivate_locked_super(sb);
+ kfree(fm);
+ return err;
+ }
+
+ sb->s_flags |= SB_ACTIVE;
+ fsc->root = dget(sb->s_root);
+ /* We are done configuring the superblock, so unlock it */
+ up_write(&sb->s_umount);
+
+ down_write(&fc->killsb);
+ list_add_tail(&fm->fc_entry, &fc->mounts);
+ up_write(&fc->killsb);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(fuse_get_tree_submount);
+
int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
{
struct fuse_dev *fud = NULL;
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index bcb8a02e2d8b..e12e5190352c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1420,6 +1420,9 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
unsigned int virtqueue_size;
int err = -EIO;
+ if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
+ return fuse_get_tree_submount(fsc);
+
/* This gets a reference on virtio_fs object. This ptr gets installed
* in fc->iq->priv. Once fuse_conn is going away, it calls ->put()
* to drop the reference to this object.
--
2.26.3
Greg Kurz
2021-May-20 15:46 UTC
[PATCH v4 3/5] fuse: Make fuse_fill_super_submount() static
This function used to be called from fuse_dentry_automount(). This code
was moved to fuse_get_tree_submount() in the same file since then. It
is unlikely there will ever be another user. No need to be extern in
this case.
Signed-off-by: Greg Kurz <groug at kaod.org>
---
fs/fuse/fuse_i.h | 9 ---------
fs/fuse/inode.c | 4 ++--
2 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index d7fcf59a6a0e..e2f5c8617e0d 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1081,15 +1081,6 @@ void fuse_send_init(struct fuse_mount *fm);
*/
int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context
*ctx);
-/*
- * Fill in superblock for submounts
- * @sb: partially-initialized superblock to fill in
- * @parent_fi: The fuse_inode of the parent filesystem where this submount is
- * mounted
- */
-int fuse_fill_super_submount(struct super_block *sb,
- struct fuse_inode *parent_fi);
-
/*
* Get the mountable root for the submount
* @fsc: superblock configuration context
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 74e5205f203c..123b53d1c3c6 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1275,8 +1275,8 @@ static void fuse_sb_defaults(struct super_block *sb)
sb->s_xattr = fuse_no_acl_xattr_handlers;
}
-int fuse_fill_super_submount(struct super_block *sb,
- struct fuse_inode *parent_fi)
+static int fuse_fill_super_submount(struct super_block *sb,
+ struct fuse_inode *parent_fi)
{
struct fuse_mount *fm = get_fuse_mount_super(sb);
struct super_block *parent_sb = parent_fi->inode.i_sb;
--
2.26.3
All submounts share the same virtio-fs device instance as the root
mount. If the same virtiofs filesystem is mounted again, sget_fc()
is likely to pick up any of these submounts and reuse it instead of
the root mount.
On the server side:
# mkdir ${some_dir}
# mkdir ${some_dir}/mnt1
# mount -t tmpfs none ${some_dir}/mnt1
# touch ${some_dir}/mnt1/THIS_IS_MNT1
# mkdir ${some_dir}/mnt2
# mount -t tmpfs none ${some_dir}/mnt2
# touch ${some_dir}/mnt2/THIS_IS_MNT2
On the client side:
# mkdir /mnt/virtiofs1
# mount -t virtiofs myfs /mnt/virtiofs1
# ls /mnt/virtiofs1
mnt1 mnt2
# grep virtiofs /proc/mounts
myfs /mnt/virtiofs1 virtiofs rw,seclabel,relatime 0 0
none on /mnt/mnt1 type virtiofs (rw,relatime,seclabel)
none on /mnt/mnt2 type virtiofs (rw,relatime,seclabel)
And now remount it again:
# mount -t virtiofs myfs /mnt/virtiofs2
# grep virtiofs /proc/mounts
myfs /mnt/virtiofs1 virtiofs rw,seclabel,relatime 0 0
none on /mnt/mnt1 type virtiofs (rw,relatime,seclabel)
none on /mnt/mnt2 type virtiofs (rw,relatime,seclabel)
myfs /mnt/virtiofs2 virtiofs rw,seclabel,relatime 0 0
# ls /mnt/virtiofs2
THIS_IS_MNT2
Submount mnt2 was picked-up instead of the root mount.
Just skip submounts in virtio_fs_test_super().
Signed-off-by: Greg Kurz <groug at kaod.org>
---
fs/fuse/virtio_fs.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index e12e5190352c..8962cd033016 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1408,6 +1408,11 @@ static int virtio_fs_test_super(struct super_block *sb,
struct fuse_mount *fsc_fm = fsc->s_fs_info;
struct fuse_mount *sb_fm = get_fuse_mount_super(sb);
+
+ /* Skip submounts */
+ if (!list_is_first(&sb_fm->fc_entry, &sb_fm->fc->mounts))
+ return 0;
+
return fsc_fm->fc->iq.priv == sb_fm->fc->iq.priv;
}
--
2.26.3
Even if POSIX doesn't mandate it, linux users legitimately expect
sync() to flush all data and metadata to physical storage when it
is located on the same system. This isn't happening with virtiofs
though : sync() inside the guest returns right away even though
data still needs to be flushed from the host page cache.
This is easily demonstrated by doing the following in the guest:
$ dd if=/dev/zero of=/mnt/foo bs=1M count=5K ; strace -T -e sync sync
5120+0 records in
5120+0 records out
5368709120 bytes (5.4 GB, 5.0 GiB) copied, 5.22224 s, 1.0 GB/s
sync() = 0 <0.024068>
+++ exited with 0 +++
and start the following in the host when the 'dd' command completes
in the guest:
$ strace -T -e fsync /usr/bin/sync virtiofs/foo
fsync(3) = 0 <10.371640>
+++ exited with 0 +++
There are no good reasons not to honor the expected behavior of
sync() actually : it gives an unrealistic impression that virtiofs
is super fast and that data has safely landed on HW, which isn't
the case obviously.
Implement a ->sync_fs() superblock operation that sends a new
FUSE_SYNCFS request type for this purpose. Provision a 64-bit
placeholder for possible future extensions. Since the file
server cannot handle the wait == 0 case, we skip it to avoid a
gratuitous roundtrip. Note that this is per-superblock : a
FUSE_SYNCFS is send for the root mount and for each submount.
Like with FUSE_FSYNC and FUSE_FSYNCDIR, lack of support for
FUSE_SYNCFS in the file server is treated as permanent success.
This ensures compatibility with older file servers : the client
will get the current behavior of sync() not being propagated to
the file server.
Note that such an operation allows the file server to DoS sync().
Since a typical FUSE file server is an untrusted piece of software
running in userspace, this is disabled by default. Only enable it
with virtiofs for now since virtiofsd is supposedly trusted by the
guest kernel.
Reported-by: Robert Krawitz <rlk at redhat.com>
Signed-off-by: Greg Kurz <groug at kaod.org>
---
fs/fuse/fuse_i.h | 3 +++
fs/fuse/inode.c | 40 +++++++++++++++++++++++++++++++++++++++
fs/fuse/virtio_fs.c | 1 +
include/uapi/linux/fuse.h | 10 +++++++++-
4 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index e2f5c8617e0d..01d9283261af 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -761,6 +761,9 @@ struct fuse_conn {
/* Auto-mount submounts announced by the server */
unsigned int auto_submounts:1;
+ /* Propagate syncfs() to server */
+ unsigned int sync_fs:1;
+
/** The number of requests waiting for completion */
atomic_t num_waiting;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 123b53d1c3c6..96b00253f766 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -506,6 +506,45 @@ static int fuse_statfs(struct dentry *dentry, struct
kstatfs *buf)
return err;
}
+static int fuse_sync_fs(struct super_block *sb, int wait)
+{
+ struct fuse_mount *fm = get_fuse_mount_super(sb);
+ struct fuse_conn *fc = fm->fc;
+ struct fuse_syncfs_in inarg;
+ FUSE_ARGS(args);
+ int err;
+
+ /*
+ * Userspace cannot handle the wait == 0 case. Avoid a
+ * gratuitous roundtrip.
+ */
+ if (!wait)
+ return 0;
+
+ /* The filesystem is being unmounted. Nothing to do. */
+ if (!sb->s_root)
+ return 0;
+
+ if (!fc->sync_fs)
+ return 0;
+
+ memset(&inarg, 0, sizeof(inarg));
+ args.in_numargs = 1;
+ args.in_args[0].size = sizeof(inarg);
+ args.in_args[0].value = &inarg;
+ args.opcode = FUSE_SYNCFS;
+ args.nodeid = get_node_id(sb->s_root->d_inode);
+ args.out_numargs = 0;
+
+ err = fuse_simple_request(fm, &args);
+ if (err == -ENOSYS) {
+ fc->sync_fs = 0;
+ err = 0;
+ }
+
+ return err;
+}
+
enum {
OPT_SOURCE,
OPT_SUBTYPE,
@@ -909,6 +948,7 @@ static const struct super_operations fuse_super_operations =
{
.put_super = fuse_put_super,
.umount_begin = fuse_umount_begin,
.statfs = fuse_statfs,
+ .sync_fs = fuse_sync_fs,
.show_options = fuse_show_options,
};
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 8962cd033016..f649a47efb68 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1455,6 +1455,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
fc->release = fuse_free_conn;
fc->delete_stale = true;
fc->auto_submounts = true;
+ fc->sync_fs = true;
/* Tell FUSE to split requests that exceed the virtqueue's size */
fc->max_pages_limit = min_t(unsigned int, fc->max_pages_limit,
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 271ae90a9bb7..36ed092227fa 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -181,6 +181,9 @@
* - add FUSE_OPEN_KILL_SUIDGID
* - extend fuse_setxattr_in, add FUSE_SETXATTR_EXT
* - add FUSE_SETXATTR_ACL_KILL_SGID
+ *
+ * 7.34
+ * - add FUSE_SYNCFS
*/
#ifndef _LINUX_FUSE_H
@@ -216,7 +219,7 @@
#define FUSE_KERNEL_VERSION 7
/** Minor version number of this interface */
-#define FUSE_KERNEL_MINOR_VERSION 33
+#define FUSE_KERNEL_MINOR_VERSION 34
/** The node ID of the root inode */
#define FUSE_ROOT_ID 1
@@ -509,6 +512,7 @@ enum fuse_opcode {
FUSE_COPY_FILE_RANGE = 47,
FUSE_SETUPMAPPING = 48,
FUSE_REMOVEMAPPING = 49,
+ FUSE_SYNCFS = 50,
/* CUSE specific operations */
CUSE_INIT = 4096,
@@ -971,4 +975,8 @@ struct fuse_removemapping_one {
#define FUSE_REMOVEMAPPING_MAX_ENTRY \
(PAGE_SIZE / sizeof(struct fuse_removemapping_one))
+struct fuse_syncfs_in {
+ uint64_t padding;
+};
+
#endif /* _LINUX_FUSE_H */
--
2.26.3