fs: Avoid grabbing sb->s_umount under bdev->bd_holder_lock

The implementation of bdev holder operations such as fs_bdev_mark_dead()
and fs_bdev_sync() grab sb->s_umount semaphore under
bdev->bd_holder_lock. This is problematic because it leads to
disk->open_mutex -> sb->s_umount lock ordering which is counterintuitive
(usually we grab higher level (e.g. filesystem) locks first and lower
level (e.g. block layer) locks later) and indeed makes lockdep complain
about possible locking cycles whenever we open a block device while
holding sb->s_umount semaphore. Implement a function
bdev_super_lock_shared() which safely transitions from holding
bdev->bd_holder_lock to holding sb->s_umount on alive superblock without
introducing the problematic lock dependency. We use this function
fs_bdev_sync() and fs_bdev_mark_dead().

Signed-off-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20231018152924.3858-1-jack@suse.cz
Link: https://lore.kernel.org/r/20231017184823.1383356-1-hch@lst.de
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christian Brauner <brauner@kernel.org>

diff --git a/block/bdev.c b/block/bdev.c
index 4628dcb1da8a58fee1d8002b0c42fdff9a109b46..9838085102b3c5a0c7a372d585c6ad8d2e4ef979 100644 (file)
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -1012,9 +1012,10 @@ void bdev_mark_dead(struct block_device *bdev, bool surprise)
        mutex_lock(&bdev->bd_holder_lock);
        if (bdev->bd_holder_ops && bdev->bd_holder_ops->mark_dead)
                bdev->bd_holder_ops->mark_dead(bdev, surprise);
-       else
+       else {
+               mutex_unlock(&bdev->bd_holder_lock);
                sync_blockdev(bdev);
-       mutex_unlock(&bdev->bd_holder_lock);
+       }
 
        invalidate_bdev(bdev);
 }

diff --git a/block/ioctl.c b/block/ioctl.c
index 5d356c96435204c46c317934ff10b9afe9fadfaf..4160f4e6bd5b4b55e540d97f6ddad7e19f605ef7 100644 (file)
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -370,9 +370,10 @@ static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
        mutex_lock(&bdev->bd_holder_lock);
        if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync)
                bdev->bd_holder_ops->sync(bdev);
-       else
+       else {
+               mutex_unlock(&bdev->bd_holder_lock);
                sync_blockdev(bdev);
-       mutex_unlock(&bdev->bd_holder_lock);
+       }
 
        invalidate_bdev(bdev);
        return 0;

diff --git a/fs/super.c b/fs/super.c
index 26b96191e9b3ca8fa5a4a8c43d3f4b8da793ec9d..799b8db1931edef43b1df1329d26b2d22ba68eb4 100644 (file)
--- a/fs/super.c
+++ b/fs/super.c
@@ -1419,32 +1419,47 @@ EXPORT_SYMBOL(sget_dev);
 
 #ifdef CONFIG_BLOCK
 /*
- * Lock a super block that the callers holds a reference to.
+ * Lock the superblock that is holder of the bdev. Returns the superblock
+ * pointer if we successfully locked the superblock and it is alive. Otherwise
+ * we return NULL and just unlock bdev->bd_holder_lock.
  *
- * The caller needs to ensure that the super_block isn't being freed while
- * calling this function, e.g. by holding a lock over the call to this function
- * and the place that clears the pointer to the superblock used by this function
- * before freeing the superblock.
+ * The function must be called with bdev->bd_holder_lock and releases it.
  */
-static bool super_lock_shared_active(struct super_block *sb)
+static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
+       __releases(&bdev->bd_holder_lock)
 {
-       bool born = super_lock_shared(sb);
+       struct super_block *sb = bdev->bd_holder;
+       bool born;
+
+       lockdep_assert_held(&bdev->bd_holder_lock);
+       lockdep_assert_not_held(&sb->s_umount);
+
+       /* Make sure sb doesn't go away from under us */
+       spin_lock(&sb_lock);
+       sb->s_count++;
+       spin_unlock(&sb_lock);
+       mutex_unlock(&bdev->bd_holder_lock);
 
+       born = super_lock_shared(sb);
        if (!born || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
                super_unlock_shared(sb);
-               return false;
+               put_super(sb);
+               return NULL;
        }
-       return true;
+       /*
+        * The superblock is active and we hold s_umount, we can drop our
+        * temporary reference now.
+        */
+       put_super(sb);
+       return sb;
 }
 
 static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
 {
-       struct super_block *sb = bdev->bd_holder;
-
-       /* bd_holder_lock ensures that the sb isn't freed */
-       lockdep_assert_held(&bdev->bd_holder_lock);
+       struct super_block *sb;
 
-       if (!super_lock_shared_active(sb))
+       sb = bdev_super_lock_shared(bdev);
+       if (!sb)
                return;
 
        if (!surprise)
@@ -1459,11 +1474,10 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
 
 static void fs_bdev_sync(struct block_device *bdev)
 {
-       struct super_block *sb = bdev->bd_holder;
-
-       lockdep_assert_held(&bdev->bd_holder_lock);
+       struct super_block *sb;
 
-       if (!super_lock_shared_active(sb))
+       sb = bdev_super_lock_shared(bdev);
+       if (!sb)
                return;
        sync_filesystem(sb);
        super_unlock_shared(sb);