Andrew Price <[email protected]> wrote: > > + up_write(&s->s_umount); > > + blkdev_put(bdev, fc->bdev_mode); > > + down_write(&s->s_umount); > > fc->bdev should be NULLed here (or, on the way out of sget_fc() might be more > appropriate) otherwise we get a double-blkdev_put() leading to NULL pointer > derefs later. This happens when I mount a device twice and then unmount them, > or mount it 3 times.
How about the attached changes? Note that they conflict a bit with the mtd changes from where I took the destructor piece. David --- commit 161602f963ae5a05bdb834461f7a4896286fbde0 Author: David Howells <[email protected]> Date: Wed Mar 27 11:12:57 2019 +0000 bdev handling fix diff --git a/fs/fs_context.c b/fs/fs_context.c index 9e22fe6aafe7..fc8fda4b9fe9 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c @@ -425,10 +425,8 @@ void put_fs_context(struct fs_context *fc) if (fc->need_free && fc->ops && fc->ops->free) fc->ops->free(fc); -#ifdef CONFIG_BLOCK - if (fc->bdev) - blkdev_put(fc->bdev, fc->bdev_mode); -#endif + if (fc->dev_destructor) + fc->dev_destructor(fc); security_free_mnt_opts(&fc->security); put_net(fc->net_ns); diff --git a/fs/super.c b/fs/super.c index 85851adb0f19..e9e678d2c37c 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1211,8 +1211,17 @@ int vfs_get_super(struct fs_context *fc, EXPORT_SYMBOL(vfs_get_super); #ifdef CONFIG_BLOCK +static void fc_bdev_destructor(struct fs_context *fc) +{ + if (fc->bdev) { + blkdev_put(fc->bdev, fc->bdev_mode); + fc->bdev = NULL; + } +} + static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc) { + s->s_mode = fc->bdev_mode; s->s_bdev = fc->bdev; s->s_dev = s->s_bdev->bd_dev; s->s_bdi = bdi_get(s->s_bdev->bd_bdi); @@ -1252,6 +1261,9 @@ int vfs_get_block_super(struct fs_context *fc, return PTR_ERR(bdev); } + fc->dev_destructor = fc_bdev_destructor; + fc->bdev = bdev; + /* Once the superblock is inserted into the list by sget_fc(), s_umount * will protect the lockfs code from trying to start a snapshot while * we are mounting @@ -1260,18 +1272,14 @@ int vfs_get_block_super(struct fs_context *fc, if (bdev->bd_fsfreeze_count > 0) { mutex_unlock(&bdev->bd_fsfreeze_mutex); warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev); - error = -EBUSY; - goto error_bdev; + return -EBUSY; } - fc->bdev = bdev; fc->sb_flags |= SB_NOSEC; s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc); mutex_unlock(&bdev->bd_fsfreeze_mutex); - if (IS_ERR(s)) { - error = PTR_ERR(s); - goto error_bdev; - } + if (IS_ERR(s)) + return PTR_ERR(s); if (s->s_root) { /* Don't summarily change the RO/RW state. */ @@ -1281,16 +1289,10 @@ int vfs_get_block_super(struct fs_context *fc, goto error_sb; } - /* s_umount nests inside bd_mutex during __invalidate_device(). - * blkdev_put() acquires bd_mutex and can't be called under - * s_umount. Drop s_umount temporarily. This is safe as we're - * holding an active reference. + /* Leave fc->bdev to fc_bdev_destructor() to clean up to avoid + * locking conflicts. */ - up_write(&s->s_umount); - blkdev_put(bdev, fc->bdev_mode); - down_write(&s->s_umount); } else { - s->s_mode = fc->bdev_mode; snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev); sb_set_blocksize(s, block_size(bdev)); error = fill_super(s, fc); @@ -1307,11 +1309,7 @@ int vfs_get_block_super(struct fs_context *fc, error_sb: deactivate_locked_super(s); -error_bdev: - if (fc->bdev) { - blkdev_put(fc->bdev, fc->bdev_mode); - fc->bdev = NULL; - } + /* Leave fc->bdev to fc_bdev_destructor() to clean up */ return error; } EXPORT_SYMBOL(vfs_get_block_super); @@ -1521,8 +1519,13 @@ int vfs_get_tree(struct fs_context *fc) * on the superblock. */ error = fc->ops->get_tree(fc); - if (error < 0) + if (error < 0) { + if (fc->dev_destructor) { + fc->dev_destructor(fc); + fc->dev_destructor = NULL; + } return error; + } if (!fc->root) { pr_err("Filesystem %s get_tree() didn't set fc->root\n", diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index cb49b92f02af..9af788a3ef8e 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -76,7 +76,9 @@ struct fs_context { const struct fs_context_operations *ops; struct file_system_type *fs_type; void *fs_private; /* The filesystem's context */ - struct block_device *bdev; /* The backing blockdev (if applicable) */ + union { + struct block_device *bdev; /* The backing blockdev (if applicable) */ + }; struct dentry *root; /* The root and superblock */ struct user_namespace *user_ns; /* The user namespace for this mount */ struct net *net_ns; /* The network namespace for this mount */ @@ -93,6 +95,7 @@ struct fs_context { enum fs_context_purpose purpose:8; bool need_free:1; /* Need to call ops->free() */ bool global:1; /* Goes into &init_user_ns */ + void (*dev_destructor)(struct fs_context *fc); /* For block or mtd */ }; struct fs_context_operations {
