| Message ID | 20230829-vfs-super-mtd-v1-2-fecb572e5df3@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: switch to keying by dev_t | expand |
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 >
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;
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(-)