[PATCH 0/2] mtd: switch to keying by dev_t

[PATCH 0/2] mtd: switch to keying by dev_t

[Christian Brauner]

Hey,

For this cycle Jan, Christoph, and myself switched the generic super
code to key superblocks for block devices by device number (sb->s_dev)
instead of block device pointers (sb->s_bdev).

Not just does this allow us to defer opening block devices after we
allocated a superblock it also allows us to move closing block devices
to a later point to avoid various deadlocks.

Similar to the generic code for block devices we need to switch mtd
devices to rely on sb->s_dev instead of sb->s_mtd to avoid potential
use-after-free issues.

I plan on taking this upstream as a fix during the merge window.

Thanks!
Christian

---



---
base-commit: dc3216b1416056b04712e53431f6e9aefdc83177
change-id: 20230829-vfs-super-mtd-1bb602abfc00

[PATCH 1/2] fs: export sget_dev()

[Christian Brauner]

They will be used for mtd devices as well.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c         | 14 ++++++++++----
 include/linux/fs.h |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index ad7ac3a24d38..88cb628de6a1 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1435,12 +1435,12 @@ static int set_bdev_super(struct super_block *s, void *data)
 	return 0;
 }
 
-static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+static int super_s_dev_set(struct super_block *s, struct fs_context *fc)
 {
 	return set_bdev_super(s, fc->sget_key);
 }
 
-static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
 {
 	return !(s->s_iflags & SB_I_RETIRED) &&
 		s->s_dev == *(dev_t *)fc->sget_key;
@@ -1500,6 +1500,13 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
 }
 EXPORT_SYMBOL_GPL(setup_bdev_super);
 
+struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
+{
+	fc->sget_key = &dev;
+	return sget_fc(fc, super_s_dev_set, super_s_dev_test);
+}
+EXPORT_SYMBOL(sget_dev);
+
 /**
  * get_tree_bdev - Get a superblock based on a single block device
  * @fc: The filesystem context holding the parameters
@@ -1523,8 +1530,7 @@ int get_tree_bdev(struct fs_context *fc,
 	}
 
 	fc->sb_flags |= SB_NOSEC;
-	fc->sget_key = &dev;
-	s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
+	s = sget_dev(fc, dev);
 	if (IS_ERR(s))
 		return PTR_ERR(s);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ca8ceccde3d6..8a8d1cd5b0a9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2274,6 +2274,7 @@ struct super_block *sget(struct file_system_type *type,
 			int (*test)(struct super_block *,void *),
 			int (*set)(struct super_block *,void *),
 			int flags, void *data);
+struct super_block *sget_dev(struct fs_context *fc, dev_t dev);
 
 /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
 #define fops_get(fops) \

-- 
2.34.1

Re: [PATCH 1/2] fs: export sget_dev()

[Christian Brauner]

> +	return sget_fc(fc, super_s_dev_set, super_s_dev_test);

return sget_fc(fc, super_s_dev_test, super_s_dev_set);

Sorry, dumb typo that I had already fixed in tree...

Re: [PATCH 1/2] fs: export sget_dev()

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Christian Brauner" <brauner@kernel.org>
> An: "hch" <hch@lst.de>, "Jan Kara" <jack@suse.cz>, "richard" <richard@nod.at>
> CC: "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, "linux-mtd"
> <linux-mtd@lists.infradead.org>, "linux-fsdevel" <linux-fsdevel@vger.kernel.org>
> Gesendet: Dienstag, 29. August 2023 18:29:16
> Betreff: Re: [PATCH 1/2] fs: export sget_dev()

>> +	return sget_fc(fc, super_s_dev_set, super_s_dev_test);
> 
> return sget_fc(fc, super_s_dev_test, super_s_dev_set);
> 
> Sorry, dumb typo that I had already fixed in tree...

What tree does this patch apply to? linux-next?
I gave it a quick try on Linus' tree but it failed too:

fs/super.c: In function ‘get_tree_bdev’:
fs/super.c:1293:19: error: ‘dev’ undeclared (first use in this function); did you mean ‘bdev’?
  s = sget_dev(fc, dev);
                   ^~~
                   bdev
fs/super.c:1293:19: note: each undeclared identifier is reported only once for each function it appears in

Thanks,
//richard

Re: [PATCH 1/2] fs: export sget_dev()

[hch]

On Tue, Aug 29, 2023 at 06:55:04PM +0200, Richard Weinberger wrote:
> What tree does this patch apply to? linux-next?
> I gave it a quick try on Linus' tree but it failed too:
> 
> fs/super.c: In function ‘get_tree_bdev’:
> fs/super.c:1293:19: error: ‘dev’ undeclared (first use in this function); did you mean ‘bdev’?
>   s = sget_dev(fc, dev);
>                    ^~~
>                    bdev
> fs/super.c:1293:19: note: each undeclared identifier is reported only once for each function it appears in

Should be against the latest Linus tree after the merge of the vfs
branches yesterday.

Re: [PATCH 1/2] fs: export sget_dev()

[Christian Brauner]

On Wed, Aug 30, 2023 at 08:13:45AM +0200, hch wrote:
> On Tue, Aug 29, 2023 at 06:55:04PM +0200, Richard Weinberger wrote:
> > What tree does this patch apply to? linux-next?
> > I gave it a quick try on Linus' tree but it failed too:
> > 
> > fs/super.c: In function ‘get_tree_bdev’:
> > fs/super.c:1293:19: error: ‘dev’ undeclared (first use in this function); did you mean ‘bdev’?
> >   s = sget_dev(fc, dev);
> >                    ^~~
> >                    bdev
> > fs/super.c:1293:19: note: each undeclared identifier is reported only once for each function it appears in
> 
> Should be against the latest Linus tree after the merge of the vfs
> branches yesterday.

I would suggest to just pull it from:
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git b4/vfs-super-mtd

Re: [PATCH 1/2] fs: export sget_dev()

[Christoph Hellwig]

> +struct super_block *sget_dev(struct fs_context *fc, dev_t dev)

A kerneldoc comment would probably be useful here.

Re: [PATCH 1/2] fs: export sget_dev()

[Christian Brauner]

On Wed, Aug 30, 2023 at 08:14:09AM +0200, Christoph Hellwig wrote:
> > +struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
> 
> A kerneldoc comment would probably be useful here.

Added the following in-treep:

diff --git a/fs/super.c b/fs/super.c
index 158e093f23c9..19fa906b118a 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1388,6 +1388,26 @@ static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
                s->s_dev == *(dev_t *)fc->sget_key;
 }

+/**
+ * sget_dev - Find or create a superblock by device number
+ * @fc:        Filesystem context.
+ * @dev: device number
+ *
+ * Find or create a superblock using the provided device number that
+ * will be stored in fc->sget_key.
+ *
+ * If an extant superblock is matched, then that will be returned with
+ * an elevated reference count that the caller must transfer or discard.
+ *
+ * If no match is made, a new superblock will be allocated and basic
+ * initialisation will be performed (s_type, s_fs_info and s_id will be
+ * set and the set() callback will be invoked), the superblock will be
+ * published and it will be returned in a partially constructed state
+ * with SB_BORN and SB_ACTIVE as yet unset.
+ *
+ * Return: an existing or newly created superblock on success, an an
+ *         error pointer on failure.
+ */
 struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
 {
        fc->sget_key = &dev;

Re: [PATCH 1/2] fs: export sget_dev()

[Jan Kara]

On Wed 30-08-23 10:05:57, Christian Brauner wrote:
> On Wed, Aug 30, 2023 at 08:14:09AM +0200, Christoph Hellwig wrote:
> > > +struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
> > 
> > A kerneldoc comment would probably be useful here.
> 
> Added the following in-treep:
> 
> diff --git a/fs/super.c b/fs/super.c
> index 158e093f23c9..19fa906b118a 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1388,6 +1388,26 @@ static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
>                 s->s_dev == *(dev_t *)fc->sget_key;
>  }
> 
> +/**
> + * sget_dev - Find or create a superblock by device number
> + * @fc:        Filesystem context.
> + * @dev: device number
      ^^^^^^ inconsistent indenting.

> + *
> + * Find or create a superblock using the provided device number that
> + * will be stored in fc->sget_key.
> + *
> + * If an extant superblock is matched, then that will be returned with
> + * an elevated reference count that the caller must transfer or discard.
> + *
> + * If no match is made, a new superblock will be allocated and basic
> + * initialisation will be performed (s_type, s_fs_info and s_id will be
> + * set and the set() callback will be invoked), the superblock will be
      ^^ I guess no point in talking about set() callback when sget_dev()
has no callback specified. Rather you could mention s_dev as one of
initialized fields.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] fs: export sget_dev()

[Christian Brauner]

On Wed, Aug 30, 2023 at 11:38:51AM +0200, Jan Kara wrote:
> On Wed 30-08-23 10:05:57, Christian Brauner wrote:
> > On Wed, Aug 30, 2023 at 08:14:09AM +0200, Christoph Hellwig wrote:
> > > > +struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
> > > 
> > > A kerneldoc comment would probably be useful here.
> > 
> > Added the following in-treep:
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index 158e093f23c9..19fa906b118a 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -1388,6 +1388,26 @@ static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
> >                 s->s_dev == *(dev_t *)fc->sget_key;
> >  }
> > 
> > +/**
> > + * sget_dev - Find or create a superblock by device number
> > + * @fc:        Filesystem context.
> > + * @dev: device number
>       ^^^^^^ inconsistent indenting.

Fixed, thanks!

> 
> > + *
> > + * Find or create a superblock using the provided device number that
> > + * will be stored in fc->sget_key.
> > + *
> > + * If an extant superblock is matched, then that will be returned with
> > + * an elevated reference count that the caller must transfer or discard.
> > + *
> > + * If no match is made, a new superblock will be allocated and basic
> > + * initialisation will be performed (s_type, s_fs_info and s_id will be
> > + * set and the set() callback will be invoked), the superblock will be
>       ^^ I guess no point in talking about set() callback when sget_dev()
> has no callback specified. Rather you could mention s_dev as one of
> initialized fields.

Yeah, good point. Done.

Re: [PATCH 1/2] fs: export sget_dev()

[Christian Brauner]

> Yeah, good point. Done.

From fe40c7fe1a87814f92f9b1d0b9fb78ac69404c33 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Tue, 29 Aug 2023 15:05:28 +0200
Subject: [PATCH 1/2] fs: export sget_dev()

They will be used for mtd devices as well.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c         | 64 ++++++++++++++++++++++++++++++++--------------
 include/linux/fs.h |  1 +
 2 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index ad7ac3a24d38..d27d80bf7c43 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1371,6 +1371,50 @@ int get_tree_keyed(struct fs_context *fc,
 }
 EXPORT_SYMBOL(get_tree_keyed);
 
+static int set_bdev_super(struct super_block *s, void *data)
+{
+	s->s_dev = *(dev_t *)data;
+	return 0;
+}
+
+static int super_s_dev_set(struct super_block *s, struct fs_context *fc)
+{
+	return set_bdev_super(s, fc->sget_key);
+}
+
+static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
+{
+	return !(s->s_iflags & SB_I_RETIRED) &&
+		s->s_dev == *(dev_t *)fc->sget_key;
+}
+
+/**
+ * sget_dev - Find or create a superblock by device number
+ * @fc: Filesystem context.
+ * @dev: device number
+ *
+ * Find or create a superblock using the provided device number that
+ * will be stored in fc->sget_key.
+ *
+ * If an extant superblock is matched, then that will be returned with
+ * an elevated reference count that the caller must transfer or discard.
+ *
+ * If no match is made, a new superblock will be allocated and basic
+ * initialisation will be performed (s_type, s_fs_info, s_id, s_dev will
+ * be set). The superblock will be published and it will be returned in
+ * a partially constructed state with SB_BORN and SB_ACTIVE as yet
+ * unset.
+ *
+ * Return: an existing or newly created superblock on success, an error
+ *         pointer on failure.
+ */
+struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
+{
+	fc->sget_key = &dev;
+	return sget_fc(fc, super_s_dev_test, super_s_dev_set);
+}
+EXPORT_SYMBOL(sget_dev);
+
 #ifdef CONFIG_BLOCK
 /*
  * Lock a super block that the callers holds a reference to.
@@ -1429,23 +1473,6 @@ const struct blk_holder_ops fs_holder_ops = {
 };
 EXPORT_SYMBOL_GPL(fs_holder_ops);
 
-static int set_bdev_super(struct super_block *s, void *data)
-{
-	s->s_dev = *(dev_t *)data;
-	return 0;
-}
-
-static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc)
-{
-	return set_bdev_super(s, fc->sget_key);
-}
-
-static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc)
-{
-	return !(s->s_iflags & SB_I_RETIRED) &&
-		s->s_dev == *(dev_t *)fc->sget_key;
-}
-
 int setup_bdev_super(struct super_block *sb, int sb_flags,
 		struct fs_context *fc)
 {
@@ -1523,8 +1550,7 @@ int get_tree_bdev(struct fs_context *fc,
 	}
 
 	fc->sb_flags |= SB_NOSEC;
-	fc->sget_key = &dev;
-	s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
+	s = sget_dev(fc, dev);
 	if (IS_ERR(s))
 		return PTR_ERR(s);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ca8ceccde3d6..8a8d1cd5b0a9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2274,6 +2274,7 @@ struct super_block *sget(struct file_system_type *type,
 			int (*test)(struct super_block *,void *),
 			int (*set)(struct super_block *,void *),
 			int flags, void *data);
+struct super_block *sget_dev(struct fs_context *fc, dev_t dev);
 
 /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
 #define fops_get(fops) \
-- 
2.34.1

Re: [PATCH 1/2] fs: export sget_dev()

[Jan Kara]

On Wed 30-08-23 12:02:47, Christian Brauner wrote:
> > Yeah, good point. Done.
> 
> From fe40c7fe1a87814f92f9b1d0b9fb78ac69404c33 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <brauner@kernel.org>
> Date: Tue, 29 Aug 2023 15:05:28 +0200
> Subject: [PATCH 1/2] fs: export sget_dev()
> 
> They will be used for mtd devices as well.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c         | 64 ++++++++++++++++++++++++++++++++--------------
>  include/linux/fs.h |  1 +
>  2 files changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index ad7ac3a24d38..d27d80bf7c43 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1371,6 +1371,50 @@ int get_tree_keyed(struct fs_context *fc,
>  }
>  EXPORT_SYMBOL(get_tree_keyed);
>  
> +static int set_bdev_super(struct super_block *s, void *data)
> +{
> +	s->s_dev = *(dev_t *)data;
> +	return 0;
> +}
> +
> +static int super_s_dev_set(struct super_block *s, struct fs_context *fc)
> +{
> +	return set_bdev_super(s, fc->sget_key);
> +}
> +
> +static int super_s_dev_test(struct super_block *s, struct fs_context *fc)
> +{
> +	return !(s->s_iflags & SB_I_RETIRED) &&
> +		s->s_dev == *(dev_t *)fc->sget_key;
> +}
> +
> +/**
> + * sget_dev - Find or create a superblock by device number
> + * @fc: Filesystem context.
> + * @dev: device number
> + *
> + * Find or create a superblock using the provided device number that
> + * will be stored in fc->sget_key.
> + *
> + * If an extant superblock is matched, then that will be returned with
> + * an elevated reference count that the caller must transfer or discard.
> + *
> + * If no match is made, a new superblock will be allocated and basic
> + * initialisation will be performed (s_type, s_fs_info, s_id, s_dev will
> + * be set). The superblock will be published and it will be returned in
> + * a partially constructed state with SB_BORN and SB_ACTIVE as yet
> + * unset.
> + *
> + * Return: an existing or newly created superblock on success, an error
> + *         pointer on failure.
> + */
> +struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
> +{
> +	fc->sget_key = &dev;
> +	return sget_fc(fc, super_s_dev_test, super_s_dev_set);
> +}
> +EXPORT_SYMBOL(sget_dev);
> +
>  #ifdef CONFIG_BLOCK
>  /*
>   * Lock a super block that the callers holds a reference to.
> @@ -1429,23 +1473,6 @@ const struct blk_holder_ops fs_holder_ops = {
>  };
>  EXPORT_SYMBOL_GPL(fs_holder_ops);
>  
> -static int set_bdev_super(struct super_block *s, void *data)
> -{
> -	s->s_dev = *(dev_t *)data;
> -	return 0;
> -}
> -
> -static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc)
> -{
> -	return set_bdev_super(s, fc->sget_key);
> -}
> -
> -static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc)
> -{
> -	return !(s->s_iflags & SB_I_RETIRED) &&
> -		s->s_dev == *(dev_t *)fc->sget_key;
> -}
> -
>  int setup_bdev_super(struct super_block *sb, int sb_flags,
>  		struct fs_context *fc)
>  {
> @@ -1523,8 +1550,7 @@ int get_tree_bdev(struct fs_context *fc,
>  	}
>  
>  	fc->sb_flags |= SB_NOSEC;
> -	fc->sget_key = &dev;
> -	s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
> +	s = sget_dev(fc, dev);
>  	if (IS_ERR(s))
>  		return PTR_ERR(s);
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ca8ceccde3d6..8a8d1cd5b0a9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2274,6 +2274,7 @@ struct super_block *sget(struct file_system_type *type,
>  			int (*test)(struct super_block *,void *),
>  			int (*set)(struct super_block *,void *),
>  			int flags, void *data);
> +struct super_block *sget_dev(struct fs_context *fc, dev_t dev);
>  
>  /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
>  #define fops_get(fops) \
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/2] mtd: key superblock by device number

[Christian Brauner]

The mtd driver has similar problems than the one that was fixed in
commit dc3216b14160 ("super: ensure valid info").

The kill_mtd_super() helper calls shuts the superblock down but leaves
the superblock on fs_supers as the devices are still in use but puts the
mtd device and cleans out the superblock's s_mtd field.

This means another mounter can find the superblock on the list accessing
its s_mtd field while it is curently in the process of being freed or
already freed.

Prevent that from happening by keying superblock by dev_t just as we do
in the generic code.

Link: https://lore.kernel.org/linux-fsdevel/20230829-weitab-lauwarm-49c40fc85863@brauner
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 drivers/mtd/mtdsuper.c | 45 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 34 deletions(-)

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index 5ff001140ef4..b7e3763c47f0 100644
--- a/drivers/mtd/mtdsuper.c
+++ b/drivers/mtd/mtdsuper.c
@@ -19,38 +19,6 @@
 #include <linux/fs_context.h>
 #include "mtdcore.h"
 
-/*
- * compare superblocks to see if they're equivalent
- * - they are if the underlying MTD device is the same
- */
-static int mtd_test_super(struct super_block *sb, struct fs_context *fc)
-{
-	struct mtd_info *mtd = fc->sget_key;
-
-	if (sb->s_mtd == fc->sget_key) {
-		pr_debug("MTDSB: Match on device %d (\"%s\")\n",
-			 mtd->index, mtd->name);
-		return 1;
-	}
-
-	pr_debug("MTDSB: No match, device %d (\"%s\"), device %d (\"%s\")\n",
-		 sb->s_mtd->index, sb->s_mtd->name, mtd->index, mtd->name);
-	return 0;
-}
-
-/*
- * mark the superblock by the MTD device it is using
- * - set the device number to be the correct MTD block device for pesuperstence
- *   of NFS exports
- */
-static int mtd_set_super(struct super_block *sb, struct fs_context *fc)
-{
-	sb->s_mtd = fc->sget_key;
-	sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index);
-	sb->s_bdi = bdi_get(mtd_bdi);
-	return 0;
-}
-
 /*
  * get a superblock on an MTD-backed filesystem
  */
@@ -62,8 +30,7 @@ static int mtd_get_sb(struct fs_context *fc,
 	struct super_block *sb;
 	int ret;
 
-	fc->sget_key = mtd;
-	sb = sget_fc(fc, mtd_test_super, mtd_set_super);
+	sb = sget_dev(fc, MKDEV(MTD_BLOCK_MAJOR, mtd->index));
 	if (IS_ERR(sb))
 		return PTR_ERR(sb);
 
@@ -77,6 +44,16 @@ static int mtd_get_sb(struct fs_context *fc,
 		pr_debug("MTDSB: New superblock for device %d (\"%s\")\n",
 			 mtd->index, mtd->name);
 
+		/*
+		 * Would usually have been set with @sb_lock held but in
+		 * contrast to sb->s_bdev that's checked with only
+		 * @sb_lock held, nothing checks sb->s_mtd without also
+		 * holding sb->s_umount and we're holding sb->s_umount
+		 * here.
+		 */
+		sb->s_mtd = mtd;
+		sb->s_bdi = bdi_get(mtd_bdi);
+
 		ret = fill_super(sb, fc);
 		if (ret < 0)
 			goto error_sb;

-- 
2.34.1

Re: [PATCH 2/2] mtd: key superblock by device number

[Jan Kara]

On Tue 29-08-23 17:23:57, Christian Brauner wrote:
> The mtd driver has similar problems than the one that was fixed in
> commit dc3216b14160 ("super: ensure valid info").
> 
> The kill_mtd_super() helper calls shuts the superblock down but leaves
> the superblock on fs_supers as the devices are still in use but puts the
> mtd device and cleans out the superblock's s_mtd field.
> 
> This means another mounter can find the superblock on the list accessing
> its s_mtd field while it is curently in the process of being freed or
> already freed.
> 
> Prevent that from happening by keying superblock by dev_t just as we do
> in the generic code.
> 
> Link: https://lore.kernel.org/linux-fsdevel/20230829-weitab-lauwarm-49c40fc85863@brauner
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/mtd/mtdsuper.c | 45 +++++++++++----------------------------------
>  1 file changed, 11 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
> index 5ff001140ef4..b7e3763c47f0 100644
> --- a/drivers/mtd/mtdsuper.c
> +++ b/drivers/mtd/mtdsuper.c
> @@ -19,38 +19,6 @@
>  #include <linux/fs_context.h>
>  #include "mtdcore.h"
>  
> -/*
> - * compare superblocks to see if they're equivalent
> - * - they are if the underlying MTD device is the same
> - */
> -static int mtd_test_super(struct super_block *sb, struct fs_context *fc)
> -{
> -	struct mtd_info *mtd = fc->sget_key;
> -
> -	if (sb->s_mtd == fc->sget_key) {
> -		pr_debug("MTDSB: Match on device %d (\"%s\")\n",
> -			 mtd->index, mtd->name);
> -		return 1;
> -	}
> -
> -	pr_debug("MTDSB: No match, device %d (\"%s\"), device %d (\"%s\")\n",
> -		 sb->s_mtd->index, sb->s_mtd->name, mtd->index, mtd->name);
> -	return 0;
> -}
> -
> -/*
> - * mark the superblock by the MTD device it is using
> - * - set the device number to be the correct MTD block device for pesuperstence
> - *   of NFS exports
> - */
> -static int mtd_set_super(struct super_block *sb, struct fs_context *fc)
> -{
> -	sb->s_mtd = fc->sget_key;
> -	sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index);
> -	sb->s_bdi = bdi_get(mtd_bdi);
> -	return 0;
> -}
> -
>  /*
>   * get a superblock on an MTD-backed filesystem
>   */
> @@ -62,8 +30,7 @@ static int mtd_get_sb(struct fs_context *fc,
>  	struct super_block *sb;
>  	int ret;
>  
> -	fc->sget_key = mtd;
> -	sb = sget_fc(fc, mtd_test_super, mtd_set_super);
> +	sb = sget_dev(fc, MKDEV(MTD_BLOCK_MAJOR, mtd->index));
>  	if (IS_ERR(sb))
>  		return PTR_ERR(sb);
>  
> @@ -77,6 +44,16 @@ static int mtd_get_sb(struct fs_context *fc,
>  		pr_debug("MTDSB: New superblock for device %d (\"%s\")\n",
>  			 mtd->index, mtd->name);
>  
> +		/*
> +		 * Would usually have been set with @sb_lock held but in
> +		 * contrast to sb->s_bdev that's checked with only
> +		 * @sb_lock held, nothing checks sb->s_mtd without also
> +		 * holding sb->s_umount and we're holding sb->s_umount
> +		 * here.
> +		 */
> +		sb->s_mtd = mtd;
> +		sb->s_bdi = bdi_get(mtd_bdi);
> +
>  		ret = fill_super(sb, fc);
>  		if (ret < 0)
>  			goto error_sb;
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] mtd: switch to keying by dev_t

[Christian Brauner]

> I plan on taking this upstream as a fix during the merge window.

Btw, I tested this all with jffs2:

sudo flash_erase -j /dev/mtd0 0 0
sudo mount -t jffs2 mtd0 /mnt

and then some variants with racing umounts and mounts.

Re: [PATCH 0/2] mtd: switch to keying by dev_t

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Christian Brauner" <brauner@kernel.org>
> An: "hch" <hch@lst.de>, "Jan Kara" <jack@suse.cz>, "richard" <richard@nod.at>
> CC: "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, "linux-mtd"
> <linux-mtd@lists.infradead.org>, "linux-fsdevel" <linux-fsdevel@vger.kernel.org>
> Gesendet: Mittwoch, 30. August 2023 11:50:58
> Betreff: Re: [PATCH 0/2] mtd: switch to keying by dev_t

>> I plan on taking this upstream as a fix during the merge window.
> 
> Btw, I tested this all with jffs2:
> 
> sudo flash_erase -j /dev/mtd0 0 0
> sudo mount -t jffs2 mtd0 /mnt
> 
> and then some variants with racing umounts and mounts.

Good to know!
Acked-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 0/2] mtd: switch to keying by dev_t

[Christoph Hellwig]

The series with all the updates from this thread looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/2] mtd: switch to keying by dev_t

[Christian Brauner]

On Tue, 29 Aug 2023 17:23:55 +0200, Christian Brauner wrote:
> Hey,
> 
> For this cycle Jan, Christoph, and myself switched the generic super
> code to key superblocks for block devices by device number (sb->s_dev)
> instead of block device pointers (sb->s_bdev).
> 
> Not just does this allow us to defer opening block devices after we
> allocated a superblock it also allows us to move closing block devices
> to a later point to avoid various deadlocks.
> 
> [...]

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[1/2] fs: export sget_dev()
      https://git.kernel.org/vfs/vfs/c/9c4d12957d16
[2/2] mtd: key superblock by device number
      https://git.kernel.org/vfs/vfs/c/ff7c9910eaad