Re: [RFC PATCH 1/4] vfs: Create fs_context-aware mount_bdev() replacement

Andrew Price <anprice@xxxxxxxxxx> · Wed, 27 Mar 2019 13:51:08 +0000

On 27/03/2019 11:23, David Howells wrote:
Andrew Price <anprice@xxxxxxxxxx> 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

It fixes my bug and I see nothing unexpected from an xfstests -g quick 
run with my gfs2 patch on top. Looks good.

Thanks,
Andy

---
commit 161602f963ae5a05bdb834461f7a4896286fbde0
Author: David Howells <dhowells@xxxxxxxxxx>
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 {







