Message ID | 20210520154654.1791183-3-groug@kaod.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | virtiofs: propagate sync() to file server | expand |
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
On Fri, 21 May 2021 10:19:48 +0200 Miklos Szeredi <miklos@szeredi.hu> wrote: > 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; > + plus the mandatory comment one must put to justify the need for a memory barrier. > /* Create the submount */ > mnt = vfs_create_mount(fsc); > if (IS_ERR(mnt)) { > > And have this patch be the cleanup. > Fair enough. > Also we need Fixes: and a Cc: stable@... tags on that one. > Oops, I'll add these in the next round. > Thanks, > Miklos
Hi Greg,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on fuse/for-next]
[also build test WARNING on linux/master linus/master v5.13-rc2 next-20210521]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Greg-Kurz/virtiofs-propagate-sync-to-file-server/20210522-210652
base: https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
config: nds32-randconfig-r011-20210522 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/ee3cc45c5a2311efc82021bd5463271507bef828
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Greg-Kurz/virtiofs-propagate-sync-to-file-server/20210522-210652
git checkout ee3cc45c5a2311efc82021bd5463271507bef828
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
fs/fuse/dir.c: In function 'fuse_dentry_automount':
>> fs/fuse/dir.c:312:21: warning: variable 'fm' set but not used [-Wunused-but-set-variable]
312 | struct fuse_mount *fm;
| ^~
vim +/fm +312 fs/fuse/dir.c
8fab010644363f Miklos Szeredi 2018-08-15 303
bf109c64040f5b Max Reitz 2020-04-21 304 /*
bf109c64040f5b Max Reitz 2020-04-21 305 * Create a fuse_mount object with a new superblock (with path->dentry
bf109c64040f5b Max Reitz 2020-04-21 306 * as the root), and return that mount so it can be auto-mounted on
bf109c64040f5b Max Reitz 2020-04-21 307 * @path.
bf109c64040f5b Max Reitz 2020-04-21 308 */
bf109c64040f5b Max Reitz 2020-04-21 309 static struct vfsmount *fuse_dentry_automount(struct path *path)
bf109c64040f5b Max Reitz 2020-04-21 310 {
bf109c64040f5b Max Reitz 2020-04-21 311 struct fs_context *fsc;
bf109c64040f5b Max Reitz 2020-04-21 @312 struct fuse_mount *fm;
bf109c64040f5b Max Reitz 2020-04-21 313 struct vfsmount *mnt;
bf109c64040f5b Max Reitz 2020-04-21 314 struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
bf109c64040f5b Max Reitz 2020-04-21 315 int err;
bf109c64040f5b Max Reitz 2020-04-21 316
bf109c64040f5b Max Reitz 2020-04-21 317 fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
bf109c64040f5b Max Reitz 2020-04-21 318 if (IS_ERR(fsc)) {
bf109c64040f5b Max Reitz 2020-04-21 319 err = PTR_ERR(fsc);
bf109c64040f5b Max Reitz 2020-04-21 320 goto out;
bf109c64040f5b Max Reitz 2020-04-21 321 }
bf109c64040f5b Max Reitz 2020-04-21 322
ee3cc45c5a2311 Greg Kurz 2021-05-20 323 /*
ee3cc45c5a2311 Greg Kurz 2021-05-20 324 * Hijack fsc->fs_private to pass the mount point inode to
ee3cc45c5a2311 Greg Kurz 2021-05-20 325 * fuse_get_tree_submount(). It *must* be NULLified afterwards
ee3cc45c5a2311 Greg Kurz 2021-05-20 326 * to avoid the inode pointer to be passed to kfree() when
ee3cc45c5a2311 Greg Kurz 2021-05-20 327 * the context gets freed.
ee3cc45c5a2311 Greg Kurz 2021-05-20 328 */
ee3cc45c5a2311 Greg Kurz 2021-05-20 329 fsc->fs_private = mp_fi;
ee3cc45c5a2311 Greg Kurz 2021-05-20 330 err = vfs_get_tree(fsc);
ee3cc45c5a2311 Greg Kurz 2021-05-20 331 fsc->fs_private = NULL;
ee3cc45c5a2311 Greg Kurz 2021-05-20 332 if (err)
bf109c64040f5b Max Reitz 2020-04-21 333 goto out_put_fsc;
bf109c64040f5b Max Reitz 2020-04-21 334
ee3cc45c5a2311 Greg Kurz 2021-05-20 335 fm = get_fuse_mount_super(fsc->root->d_sb);
bf109c64040f5b Max Reitz 2020-04-21 336
bf109c64040f5b Max Reitz 2020-04-21 337 /* Create the submount */
bf109c64040f5b Max Reitz 2020-04-21 338 mnt = vfs_create_mount(fsc);
bf109c64040f5b Max Reitz 2020-04-21 339 if (IS_ERR(mnt)) {
bf109c64040f5b Max Reitz 2020-04-21 340 err = PTR_ERR(mnt);
bf109c64040f5b Max Reitz 2020-04-21 341 goto out_put_fsc;
bf109c64040f5b Max Reitz 2020-04-21 342 }
bf109c64040f5b Max Reitz 2020-04-21 343 mntget(mnt);
bf109c64040f5b Max Reitz 2020-04-21 344 put_fs_context(fsc);
bf109c64040f5b Max Reitz 2020-04-21 345 return mnt;
bf109c64040f5b Max Reitz 2020-04-21 346
bf109c64040f5b Max Reitz 2020-04-21 347 out_put_fsc:
bf109c64040f5b Max Reitz 2020-04-21 348 put_fs_context(fsc);
bf109c64040f5b Max Reitz 2020-04-21 349 out:
bf109c64040f5b Max Reitz 2020-04-21 350 return ERR_PTR(err);
bf109c64040f5b Max Reitz 2020-04-21 351 }
bf109c64040f5b Max Reitz 2020-04-21 352
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Greg,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on fuse/for-next]
[also build test WARNING on linux/master linus/master v5.13-rc2 next-20210521]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Greg-Kurz/virtiofs-propagate-sync-to-file-server/20210522-210652
base: https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
config: sparc64-randconfig-p002-20210522 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/ee3cc45c5a2311efc82021bd5463271507bef828
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Greg-Kurz/virtiofs-propagate-sync-to-file-server/20210522-210652
git checkout ee3cc45c5a2311efc82021bd5463271507bef828
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
fs/fuse/dir.c: In function 'fuse_dentry_automount':
>> fs/fuse/dir.c:312:21: warning: variable 'fm' set but not used [-Wunused-but-set-variable]
312 | struct fuse_mount *fm;
| ^~
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for FRAME_POINTER
Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS || MCOUNT
Selected by
- LOCKDEP && DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && !MIPS && !PPC && !ARM && !S390 && !MICROBLAZE && !ARC && !X86
vim +/fm +312 fs/fuse/dir.c
8fab010644363f Miklos Szeredi 2018-08-15 303
bf109c64040f5b Max Reitz 2020-04-21 304 /*
bf109c64040f5b Max Reitz 2020-04-21 305 * Create a fuse_mount object with a new superblock (with path->dentry
bf109c64040f5b Max Reitz 2020-04-21 306 * as the root), and return that mount so it can be auto-mounted on
bf109c64040f5b Max Reitz 2020-04-21 307 * @path.
bf109c64040f5b Max Reitz 2020-04-21 308 */
bf109c64040f5b Max Reitz 2020-04-21 309 static struct vfsmount *fuse_dentry_automount(struct path *path)
bf109c64040f5b Max Reitz 2020-04-21 310 {
bf109c64040f5b Max Reitz 2020-04-21 311 struct fs_context *fsc;
bf109c64040f5b Max Reitz 2020-04-21 @312 struct fuse_mount *fm;
bf109c64040f5b Max Reitz 2020-04-21 313 struct vfsmount *mnt;
bf109c64040f5b Max Reitz 2020-04-21 314 struct fuse_inode *mp_fi = get_fuse_inode(d_inode(path->dentry));
bf109c64040f5b Max Reitz 2020-04-21 315 int err;
bf109c64040f5b Max Reitz 2020-04-21 316
bf109c64040f5b Max Reitz 2020-04-21 317 fsc = fs_context_for_submount(path->mnt->mnt_sb->s_type, path->dentry);
bf109c64040f5b Max Reitz 2020-04-21 318 if (IS_ERR(fsc)) {
bf109c64040f5b Max Reitz 2020-04-21 319 err = PTR_ERR(fsc);
bf109c64040f5b Max Reitz 2020-04-21 320 goto out;
bf109c64040f5b Max Reitz 2020-04-21 321 }
bf109c64040f5b Max Reitz 2020-04-21 322
ee3cc45c5a2311 Greg Kurz 2021-05-20 323 /*
ee3cc45c5a2311 Greg Kurz 2021-05-20 324 * Hijack fsc->fs_private to pass the mount point inode to
ee3cc45c5a2311 Greg Kurz 2021-05-20 325 * fuse_get_tree_submount(). It *must* be NULLified afterwards
ee3cc45c5a2311 Greg Kurz 2021-05-20 326 * to avoid the inode pointer to be passed to kfree() when
ee3cc45c5a2311 Greg Kurz 2021-05-20 327 * the context gets freed.
ee3cc45c5a2311 Greg Kurz 2021-05-20 328 */
ee3cc45c5a2311 Greg Kurz 2021-05-20 329 fsc->fs_private = mp_fi;
ee3cc45c5a2311 Greg Kurz 2021-05-20 330 err = vfs_get_tree(fsc);
ee3cc45c5a2311 Greg Kurz 2021-05-20 331 fsc->fs_private = NULL;
ee3cc45c5a2311 Greg Kurz 2021-05-20 332 if (err)
bf109c64040f5b Max Reitz 2020-04-21 333 goto out_put_fsc;
bf109c64040f5b Max Reitz 2020-04-21 334
ee3cc45c5a2311 Greg Kurz 2021-05-20 335 fm = get_fuse_mount_super(fsc->root->d_sb);
bf109c64040f5b Max Reitz 2020-04-21 336
bf109c64040f5b Max Reitz 2020-04-21 337 /* Create the submount */
bf109c64040f5b Max Reitz 2020-04-21 338 mnt = vfs_create_mount(fsc);
bf109c64040f5b Max Reitz 2020-04-21 339 if (IS_ERR(mnt)) {
bf109c64040f5b Max Reitz 2020-04-21 340 err = PTR_ERR(mnt);
bf109c64040f5b Max Reitz 2020-04-21 341 goto out_put_fsc;
bf109c64040f5b Max Reitz 2020-04-21 342 }
bf109c64040f5b Max Reitz 2020-04-21 343 mntget(mnt);
bf109c64040f5b Max Reitz 2020-04-21 344 put_fs_context(fsc);
bf109c64040f5b Max Reitz 2020-04-21 345 return mnt;
bf109c64040f5b Max Reitz 2020-04-21 346
bf109c64040f5b Max Reitz 2020-04-21 347 out_put_fsc:
bf109c64040f5b Max Reitz 2020-04-21 348 put_fs_context(fsc);
bf109c64040f5b Max Reitz 2020-04-21 349 out:
bf109c64040f5b Max Reitz 2020-04-21 350 return ERR_PTR(err);
bf109c64040f5b Max Reitz 2020-04-21 351 }
bf109c64040f5b Max Reitz 2020-04-21 352
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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.
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@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(-)