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@xxxxxxxxxx>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
