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>

authored by

Jan Kara and committed by

Christian Brauner 2 years ago fd146410 6306ff39

+38 -22

3 changed files

expand all

block

bdev.c

ioctl.c

super.c

+3 -2

block/bdev.c

··· 1012 1012 mutex_lock(&bdev->bd_holder_lock); 1013 1013 if (bdev->bd_holder_ops && bdev->bd_holder_ops->mark_dead) 1014 1014 bdev->bd_holder_ops->mark_dead(bdev, surprise); 1015 - else 1015 + else { 1016 + mutex_unlock(&bdev->bd_holder_lock); 1016 1017 sync_blockdev(bdev); 1017 - mutex_unlock(&bdev->bd_holder_lock); 1018 + } 1018 1019 1019 1020 invalidate_bdev(bdev); 1020 1021 }

+3 -2

block/ioctl.c

··· 370 370 mutex_lock(&bdev->bd_holder_lock); 371 371 if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync) 372 372 bdev->bd_holder_ops->sync(bdev); 373 - else 373 + else { 374 + mutex_unlock(&bdev->bd_holder_lock); 374 375 sync_blockdev(bdev); 375 - mutex_unlock(&bdev->bd_holder_lock); 376 + } 376 377 377 378 invalidate_bdev(bdev); 378 379 return 0;

+32 -18

fs/super.c

··· 1419 1419 1420 1420 #ifdef CONFIG_BLOCK 1421 1421 /* 1422 - * Lock a super block that the callers holds a reference to. 1422 + * Lock the superblock that is holder of the bdev. Returns the superblock 1423 + * pointer if we successfully locked the superblock and it is alive. Otherwise 1424 + * we return NULL and just unlock bdev->bd_holder_lock. 1423 1425 * 1424 - * The caller needs to ensure that the super_block isn't being freed while 1425 - * calling this function, e.g. by holding a lock over the call to this function 1426 - * and the place that clears the pointer to the superblock used by this function 1427 - * before freeing the superblock. 1426 + * The function must be called with bdev->bd_holder_lock and releases it. 1428 1427 */ 1429 - static bool super_lock_shared_active(struct super_block *sb) 1428 + static struct super_block *bdev_super_lock_shared(struct block_device *bdev) 1429 + __releases(&bdev->bd_holder_lock) 1430 1430 { 1431 - bool born = super_lock_shared(sb); 1431 + struct super_block *sb = bdev->bd_holder; 1432 + bool born; 1432 1433 1434 + lockdep_assert_held(&bdev->bd_holder_lock); 1435 + lockdep_assert_not_held(&sb->s_umount); 1436 + 1437 + /* Make sure sb doesn't go away from under us */ 1438 + spin_lock(&sb_lock); 1439 + sb->s_count++; 1440 + spin_unlock(&sb_lock); 1441 + mutex_unlock(&bdev->bd_holder_lock); 1442 + 1443 + born = super_lock_shared(sb); 1433 1444 if (!born || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) { 1434 1445 super_unlock_shared(sb); 1435 - return false; 1446 + put_super(sb); 1447 + return NULL; 1436 1448 } 1437 - return true; 1449 + /* 1450 + * The superblock is active and we hold s_umount, we can drop our 1451 + * temporary reference now. 1452 + */ 1453 + put_super(sb); 1454 + return sb; 1438 1455 } 1439 1456 1440 1457 static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise) 1441 1458 { 1442 - struct super_block *sb = bdev->bd_holder; 1459 + struct super_block *sb; 1443 1460 1444 - /* bd_holder_lock ensures that the sb isn't freed */ 1445 - lockdep_assert_held(&bdev->bd_holder_lock); 1446 - 1447 - if (!super_lock_shared_active(sb)) 1461 + sb = bdev_super_lock_shared(bdev); 1462 + if (!sb) 1448 1463 return; 1449 1464 1450 1465 if (!surprise) ··· 1474 1459 1475 1460 static void fs_bdev_sync(struct block_device *bdev) 1476 1461 { 1477 - struct super_block *sb = bdev->bd_holder; 1462 + struct super_block *sb; 1478 1463 1479 - lockdep_assert_held(&bdev->bd_holder_lock); 1480 - 1481 - if (!super_lock_shared_active(sb)) 1464 + sb = bdev_super_lock_shared(bdev); 1465 + if (!sb) 1482 1466 return; 1483 1467 sync_filesystem(sb); 1484 1468 super_unlock_shared(sb);

Configure Feed

Configure Feed