All of lore.kernel.org
 help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Greg Kurz <groug@kaod.org>
Cc: virtualization@lists.linux-foundation.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	virtio-fs@redhat.com, Stefan Hajnoczi <stefanha@redhat.com>,
	Max Reitz <mreitz@redhat.com>, Vivek Goyal <vgoyal@redhat.com>
Subject: Re: [PATCH v4 2/5] fuse: Call vfs_get_tree() for submounts
Date: Fri, 21 May 2021 10:19:48 +0200	[thread overview]
Message-ID: <YKdtJCo/06q594pM@miu.piliscsaba.redhat.com> (raw)
In-Reply-To: <20210520154654.1791183-3-groug@kaod.org>

On Thu, May 20, 2021 at 05:46:51PM +0200, Greg Kurz wrote:
> 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.

Looks correct, but...

as an easily backportable and verifiable bugfix I'd still go with the
simple two liner:

--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -351,6 +351,9 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 	list_add_tail(&fm->fc_entry, &fc->mounts);
 	up_write(&fc->killsb);
 
+	smp_wmb();
+	sb->s_flags |= SB_BORN;
+
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
 	if (IS_ERR(mnt)) {

And have this patch be the cleanup.

Also we need Fixes: and a Cc: stable@... tags on that one.

Thanks,
Miklos

WARNING: multiple messages have this Message-ID (diff)
From: Miklos Szeredi <miklos@szeredi.hu>
To: Greg Kurz <groug@kaod.org>
Cc: linux-kernel@vger.kernel.org, Max Reitz <mreitz@redhat.com>,
	virtio-fs@redhat.com, linux-fsdevel@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	Vivek Goyal <vgoyal@redhat.com>
Subject: Re: [Virtio-fs] [PATCH v4 2/5] fuse: Call vfs_get_tree() for submounts
Date: Fri, 21 May 2021 10:19:48 +0200	[thread overview]
Message-ID: <YKdtJCo/06q594pM@miu.piliscsaba.redhat.com> (raw)
In-Reply-To: <20210520154654.1791183-3-groug@kaod.org>

On Thu, May 20, 2021 at 05:46:51PM +0200, Greg Kurz wrote:
> 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.

Looks correct, but...

as an easily backportable and verifiable bugfix I'd still go with the
simple two liner:

--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -351,6 +351,9 @@ static struct vfsmount *fuse_dentry_automount(struct path *path)
 	list_add_tail(&fm->fc_entry, &fc->mounts);
 	up_write(&fc->killsb);
 
+	smp_wmb();
+	sb->s_flags |= SB_BORN;
+
 	/* Create the submount */
 	mnt = vfs_create_mount(fsc);
 	if (IS_ERR(mnt)) {

And have this patch be the cleanup.

Also we need Fixes: and a Cc: stable@... tags on that one.

Thanks,
Miklos


  reply	other threads:[~2021-05-21  8:19 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20 15:46 [PATCH v4 0/5] virtiofs: propagate sync() to file server Greg Kurz
2021-05-20 15:46 ` [Virtio-fs] " Greg Kurz
2021-05-20 15:46 ` Greg Kurz
2021-05-20 15:46 ` [PATCH v4 1/5] fuse: Fix leak in fuse_dentry_automount() error path Greg Kurz
2021-05-20 15:46   ` [Virtio-fs] " Greg Kurz
2021-05-20 15:46   ` Greg Kurz
2021-05-20 19:45   ` Al Viro
2021-05-20 19:45     ` [Virtio-fs] " Al Viro
2021-05-20 19:45     ` Al Viro
2021-05-21  7:54     ` Miklos Szeredi
2021-05-21  7:54       ` [Virtio-fs] " Miklos Szeredi
2021-05-21  8:15       ` Greg Kurz
2021-05-21  8:15         ` [Virtio-fs] " Greg Kurz
2021-05-21  8:15         ` Greg Kurz
2021-05-21  8:23         ` Miklos Szeredi
2021-05-21  8:23           ` [Virtio-fs] " Miklos Szeredi
2021-05-21  8:08     ` Greg Kurz
2021-05-21  8:08       ` [Virtio-fs] " Greg Kurz
2021-05-21  8:08       ` Greg Kurz
2021-05-20 15:46 ` [PATCH v4 2/5] fuse: Call vfs_get_tree() for submounts Greg Kurz
2021-05-20 15:46   ` [Virtio-fs] " Greg Kurz
2021-05-20 15:46   ` Greg Kurz
2021-05-21  8:19   ` Miklos Szeredi [this message]
2021-05-21  8:19     ` [Virtio-fs] " Miklos Szeredi
2021-05-21  8:28     ` Greg Kurz
2021-05-21  8:28       ` [Virtio-fs] " Greg Kurz
2021-05-21  8:28       ` Greg Kurz
2021-05-22 17:50   ` kernel test robot
2021-05-22 17:50     ` [Virtio-fs] " kernel test robot
2021-05-22 17:50     ` kernel test robot
2021-05-22 17:50     ` kernel test robot
2021-05-22 20:12   ` kernel test robot
2021-05-22 20:12     ` [Virtio-fs] " kernel test robot
2021-05-22 20:12     ` kernel test robot
2021-05-22 20:12     ` kernel test robot
2021-05-20 15:46 ` [PATCH v4 3/5] fuse: Make fuse_fill_super_submount() static Greg Kurz
2021-05-20 15:46   ` [Virtio-fs] " Greg Kurz
2021-05-20 15:46   ` Greg Kurz
2021-05-20 15:46 ` [PATCH v4 4/5] virtiofs: Skip submounts in sget_fc() Greg Kurz
2021-05-20 15:46   ` [Virtio-fs] " Greg Kurz
2021-05-20 15:46   ` Greg Kurz
2021-05-21  8:26   ` Miklos Szeredi
2021-05-21  8:26     ` [Virtio-fs] " Miklos Szeredi
2021-05-21  8:39     ` Greg Kurz
2021-05-21  8:39       ` [Virtio-fs] " Greg Kurz
2021-05-21  8:39       ` Greg Kurz
2021-05-21  8:50       ` Miklos Szeredi
2021-05-21  8:50         ` [Virtio-fs] " Miklos Szeredi
2021-05-21 10:06         ` Greg Kurz
2021-05-21 10:06           ` [Virtio-fs] " Greg Kurz
2021-05-21 10:06           ` Greg Kurz
2021-05-21 12:37           ` Miklos Szeredi
2021-05-21 12:37             ` [Virtio-fs] " Miklos Szeredi
2021-05-21 13:36             ` Greg Kurz
2021-05-21 13:36               ` [Virtio-fs] " Greg Kurz
2021-05-21 13:36               ` Greg Kurz
2021-05-20 15:46 ` [PATCH v4 5/5] virtiofs: propagate sync() to file server Greg Kurz
2021-05-20 15:46   ` [Virtio-fs] " Greg Kurz
2021-05-20 15:46   ` Greg Kurz
2021-05-21 10:08   ` Greg Kurz
2021-05-21 10:08     ` [Virtio-fs] " Greg Kurz
2021-05-21 10:08     ` Greg Kurz
2021-05-21 12:51     ` Miklos Szeredi
2021-05-21 12:51       ` [Virtio-fs] " Miklos Szeredi
2021-08-15 14:14   ` Amir Goldstein
2021-08-15 14:14     ` [Virtio-fs] " Amir Goldstein
2021-08-16 15:29     ` Vivek Goyal
2021-08-16 15:29       ` [Virtio-fs] " Vivek Goyal
2021-08-16 15:29       ` Vivek Goyal
2021-08-16 18:57       ` Amir Goldstein
2021-08-16 18:57         ` [Virtio-fs] " Amir Goldstein
2021-08-16 19:11         ` Vivek Goyal
2021-08-16 19:11           ` [Virtio-fs] " Vivek Goyal
2021-08-16 19:11           ` Vivek Goyal
2021-08-16 19:46           ` Amir Goldstein
2021-08-16 19:46             ` [Virtio-fs] " Amir Goldstein
2021-08-28 15:21       ` Miklos Szeredi
2021-08-28 15:21         ` [Virtio-fs] " Miklos Szeredi
2021-08-30 17:01         ` Vivek Goyal
2021-08-30 17:01           ` [Virtio-fs] " Vivek Goyal
2021-08-30 17:01           ` Vivek Goyal
2021-08-30 17:36           ` Miklos Szeredi
2021-08-30 17:36             ` [Virtio-fs] " Miklos Szeredi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YKdtJCo/06q594pM@miu.piliscsaba.redhat.com \
    --to=miklos@szeredi.hu \
    --cc=groug@kaod.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mreitz@redhat.com \
    --cc=stefanha@redhat.com \
    --cc=vgoyal@redhat.com \
    --cc=virtio-fs@redhat.com \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.