don't take s_umount under open_mutex

don't take s_umount under open_mutex

[Christoph Hellwig]

Hi all,

Christian has been pestering Jan and me a bit about finally fixing
all the pre-existing mostly theoretical cases of s_umount taken under
open_mutex.  This series, which is mostly from him with some help from
me should get us to that goal by replacing bdev_mark_dead calls that
can't ever reach a file system holder to call into with simple bdev
page invalidation.

Expect future version to come from Christian again, I'm just helping
out while he is trouble shooting his mail setup.

Diffstat:
 block/disk-events.c     |   18 +++++++-----------
 block/genhd.c           |    7 +++++++
 block/partitions/core.c |   43 +++++++++++++++++++++++++++++--------------
 drivers/block/ataflop.c |    4 +++-
 drivers/block/floppy.c  |    4 +++-
 fs/super.c              |    2 ++
 6 files changed, 51 insertions(+), 27 deletions(-)

[PATCH 1/5] block: simplify bdev_del_partition()

[Christoph Hellwig]

From: Christian Brauner <brauner@kernel.org>

BLKPG_DEL_PARTITION refuses to delete partitions that still have
openers, i.e., that has an elevated @bdev->bd_openers count. If a device
is claimed by setting @bdev->bd_holder and @bdev->bd_holder_ops
@bdev->bd_openers and @bdev->bd_holders are incremented.
@bdev->bd_openers is effectively guaranteed to be >= @bdev->bd_holders.
So as long as @bdev->bd_openers isn't zero we know that this partition
is still in active use and that there might still be @bdev->bd_holder
and @bdev->bd_holder_ops set.

The only current example is @fs_holder_ops for filesystems. But that
means bdev_mark_dead() which calls into
bdev->bd_holder_ops->mark_dead::fs_bdev_mark_dead() is a nop. As long as
there's an elevated @bdev->bd_openers count we can't delete the
partition and if there isn't an elevated @bdev->bd_openers count then
there's no @bdev->bd_holder or @bdev->bd_holder_ops.

So simply open-code what we need to do. This gets rid of one more
instance where we acquire s_umount under @disk->open_mutex.

Link: https://lore.kernel.org/r/20231016-fototermin-umriss-59f1ea6c1fe6@brauner
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/partitions/core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index e137a87f4db0d3..b0585536b407a5 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -485,7 +485,18 @@ int bdev_del_partition(struct gendisk *disk, int partno)
 	if (atomic_read(&part->bd_openers))
 		goto out_unlock;
 
-	delete_partition(part);
+	/*
+	 * We verified that @part->bd_openers is zero above and so
+	 * @part->bd_holder{_ops} can't be set. And since we hold
+	 * @disk->open_mutex the device can't be claimed by anyone.
+	 *
+	 * So no need to call @part->bd_holder_ops->mark_dead() here.
+	 * Just delete the partition and invalidate it.
+	 */
+
+	remove_inode_hash(part->bd_inode);
+	invalidate_bdev(part);
+	drop_partition(part);
 	ret = 0;
 out_unlock:
 	mutex_unlock(&disk->open_mutex);
-- 
2.39.2

Re: [PATCH 1/5] block: simplify bdev_del_partition()

[Ming Lei]

On Tue, Oct 17, 2023 at 08:48:19PM +0200, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> BLKPG_DEL_PARTITION refuses to delete partitions that still have
> openers, i.e., that has an elevated @bdev->bd_openers count. If a device
> is claimed by setting @bdev->bd_holder and @bdev->bd_holder_ops
> @bdev->bd_openers and @bdev->bd_holders are incremented.
> @bdev->bd_openers is effectively guaranteed to be >= @bdev->bd_holders.
> So as long as @bdev->bd_openers isn't zero we know that this partition
> is still in active use and that there might still be @bdev->bd_holder
> and @bdev->bd_holder_ops set.
> 
> The only current example is @fs_holder_ops for filesystems. But that
> means bdev_mark_dead() which calls into
> bdev->bd_holder_ops->mark_dead::fs_bdev_mark_dead() is a nop. As long as
> there's an elevated @bdev->bd_openers count we can't delete the
> partition and if there isn't an elevated @bdev->bd_openers count then
> there's no @bdev->bd_holder or @bdev->bd_holder_ops.
> 
> So simply open-code what we need to do. This gets rid of one more
> instance where we acquire s_umount under @disk->open_mutex.
> 
> Link: https://lore.kernel.org/r/20231016-fototermin-umriss-59f1ea6c1fe6@brauner
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

inc/dec(part->bd_openers) is always done with ->open_mutex held, so this
change is correct.

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

[PATCH 2/5] block: WARN_ON_ONCE() when we remove active partitions

[Christoph Hellwig]

From: Christian Brauner <brauner@kernel.org>

The logic for disk->open_partitions is:

blkdev_get_by_*()
-> bdev_is_partition()
   -> blkdev_get_part()
      -> blkdev_get_whole() // bdev_whole->bd_openers++
      -> if (part->bd_openers == 0)
                 disk->open_partitions++
         part->bd_openers

In other words, when we first claim/open a partition we increment
disk->open_partitions and only when all part->bd_openers are closed will
disk->open_partitions be zero. That should mean that
disk->open_partitions is always > 0 as long as there's anyone that
has an open partition.

So the check for disk->open_partitions should meand that we can never
remove an active partition that has a holder and holder ops set. Assert
that in the code. The main disk isn't removed so that check doesn't work
for disk->part0 which is what we want. After all we only care about
partition not about the main disk.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 block/partitions/core.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index b0585536b407a5..f47ffcfdfcec22 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -274,17 +274,6 @@ void drop_partition(struct block_device *part)
 	put_device(&part->bd_device);
 }
 
-static void delete_partition(struct block_device *part)
-{
-	/*
-	 * Remove the block device from the inode hash, so that it cannot be
-	 * looked up any more even when openers still hold references.
-	 */
-	remove_inode_hash(part->bd_inode);
-	bdev_mark_dead(part, false);
-	drop_partition(part);
-}
-
 static ssize_t whole_disk_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
@@ -674,8 +663,23 @@ int bdev_disk_changed(struct gendisk *disk, bool invalidate)
 	sync_blockdev(disk->part0);
 	invalidate_bdev(disk->part0);
 
-	xa_for_each_start(&disk->part_tbl, idx, part, 1)
-		delete_partition(part);
+	xa_for_each_start(&disk->part_tbl, idx, part, 1) {
+		/*
+		 * Remove the block device from the inode hash, so that
+		 * it cannot be looked up any more even when openers
+		 * still hold references.
+		 */
+		remove_inode_hash(part->bd_inode);
+
+		/*
+		 * If @disk->open_partitions isn't elevated but there's
+		 * still an active holder of that block device things
+		 * are broken.
+		 */
+		WARN_ON_ONCE(atomic_read(&part->bd_openers));
+		invalidate_bdev(part);
+		drop_partition(part);
+	}
 	clear_bit(GD_NEED_PART_SCAN, &disk->state);
 
 	/*
-- 
2.39.2

Re: [PATCH 2/5] block: WARN_ON_ONCE() when we remove active partitions

[Ming Lei]

On Tue, Oct 17, 2023 at 08:48:20PM +0200, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> The logic for disk->open_partitions is:
> 
> blkdev_get_by_*()
> -> bdev_is_partition()
>    -> blkdev_get_part()
>       -> blkdev_get_whole() // bdev_whole->bd_openers++
>       -> if (part->bd_openers == 0)
>                  disk->open_partitions++
>          part->bd_openers
> 
> In other words, when we first claim/open a partition we increment
> disk->open_partitions and only when all part->bd_openers are closed will
> disk->open_partitions be zero. That should mean that
> disk->open_partitions is always > 0 as long as there's anyone that
> has an open partition.
> 
> So the check for disk->open_partitions should meand that we can never
> remove an active partition that has a holder and holder ops set. Assert
> that in the code. The main disk isn't removed so that check doesn't work
> for disk->part0 which is what we want. After all we only care about
> partition not about the main disk.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

inc/dec(part->bd_openers) is always done with ->open_mutex held, so this
change is correct.

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

Re: [PATCH 2/5] block: WARN_ON_ONCE() when we remove active partitions

[Jan Kara]

On Tue 17-10-23 20:48:20, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> The logic for disk->open_partitions is:
> 
> blkdev_get_by_*()
> -> bdev_is_partition()
>    -> blkdev_get_part()
>       -> blkdev_get_whole() // bdev_whole->bd_openers++
>       -> if (part->bd_openers == 0)
>                  disk->open_partitions++
>          part->bd_openers
> 
> In other words, when we first claim/open a partition we increment
> disk->open_partitions and only when all part->bd_openers are closed will
> disk->open_partitions be zero. That should mean that
> disk->open_partitions is always > 0 as long as there's anyone that
> has an open partition.
> 
> So the check for disk->open_partitions should meand that we can never
> remove an active partition that has a holder and holder ops set. Assert
> that in the code. The main disk isn't removed so that check doesn't work
> for disk->part0 which is what we want. After all we only care about
> partition not about the main disk.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/partitions/core.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/block/partitions/core.c b/block/partitions/core.c
> index b0585536b407a5..f47ffcfdfcec22 100644
> --- a/block/partitions/core.c
> +++ b/block/partitions/core.c
> @@ -274,17 +274,6 @@ void drop_partition(struct block_device *part)
>  	put_device(&part->bd_device);
>  }
>  
> -static void delete_partition(struct block_device *part)
> -{
> -	/*
> -	 * Remove the block device from the inode hash, so that it cannot be
> -	 * looked up any more even when openers still hold references.
> -	 */
> -	remove_inode_hash(part->bd_inode);
> -	bdev_mark_dead(part, false);
> -	drop_partition(part);
> -}
> -
>  static ssize_t whole_disk_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
> @@ -674,8 +663,23 @@ int bdev_disk_changed(struct gendisk *disk, bool invalidate)
>  	sync_blockdev(disk->part0);
>  	invalidate_bdev(disk->part0);
>  
> -	xa_for_each_start(&disk->part_tbl, idx, part, 1)
> -		delete_partition(part);
> +	xa_for_each_start(&disk->part_tbl, idx, part, 1) {
> +		/*
> +		 * Remove the block device from the inode hash, so that
> +		 * it cannot be looked up any more even when openers
> +		 * still hold references.
> +		 */
> +		remove_inode_hash(part->bd_inode);
> +
> +		/*
> +		 * If @disk->open_partitions isn't elevated but there's
> +		 * still an active holder of that block device things
> +		 * are broken.
> +		 */
> +		WARN_ON_ONCE(atomic_read(&part->bd_openers));
> +		invalidate_bdev(part);
> +		drop_partition(part);
> +	}
>  	clear_bit(GD_NEED_PART_SCAN, &disk->state);
>  
>  	/*
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Christoph Hellwig]

disk_check_media_change is mostly called from ->open where it makes
little sense to mark the file system on the device as dead, as we
are just opening it.  So instead of calling bdev_mark_dead from
disk_check_media_change move it into the few callers that are not
in an open instance.  This avoid calling into bdev_mark_dead and
thus taking s_umount with open_mutex held.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/disk-events.c     | 18 +++++++-----------
 drivers/block/ataflop.c |  4 +++-
 drivers/block/floppy.c  |  4 +++-
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/block/disk-events.c b/block/disk-events.c
index 13c3372c465a39..2f697224386aa8 100644
--- a/block/disk-events.c
+++ b/block/disk-events.c
@@ -266,11 +266,8 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask)
  * disk_check_media_change - check if a removable media has been changed
  * @disk: gendisk to check
  *
- * Check whether a removable media has been changed, and attempt to free all
- * dentries and inodes and invalidates all block device page cache entries in
- * that case.
- *
- * Returns %true if the media has changed, or %false if not.
+ * Returns %true and marks the disk for a partition rescan whether a removable
+ * media has been changed, and %false if the media did not change.
  */
 bool disk_check_media_change(struct gendisk *disk)
 {
@@ -278,12 +275,11 @@ bool disk_check_media_change(struct gendisk *disk)
 
 	events = disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE |
 				   DISK_EVENT_EJECT_REQUEST);
-	if (!(events & DISK_EVENT_MEDIA_CHANGE))
-		return false;
-
-	bdev_mark_dead(disk->part0, true);
-	set_bit(GD_NEED_PART_SCAN, &disk->state);
-	return true;
+	if (events & DISK_EVENT_MEDIA_CHANGE) {
+		set_bit(GD_NEED_PART_SCAN, &disk->state);
+		return true;
+	}
+	return false;
 }
 EXPORT_SYMBOL(disk_check_media_change);
 
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index cd738cab725f39..50949207798d2a 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1760,8 +1760,10 @@ static int fd_locked_ioctl(struct block_device *bdev, blk_mode_t mode,
 		/* invalidate the buffer track to force a reread */
 		BufferDrive = -1;
 		set_bit(drive, &fake_change);
-		if (disk_check_media_change(disk))
+		if (disk_check_media_change(disk)) {
+			bdev_mark_dead(disk->part0, true);
 			floppy_revalidate(disk);
+		}
 		return 0;
 	default:
 		return -EINVAL;
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index ea4eb88a2e45f4..11114a5d9e5c46 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3215,8 +3215,10 @@ static int invalidate_drive(struct gendisk *disk)
 	/* invalidate the buffer track to force a reread */
 	set_bit((long)disk->private_data, &fake_change);
 	process_fd_request();
-	if (disk_check_media_change(disk))
+	if (disk_check_media_change(disk)) {
+		bdev_mark_dead(disk->part0, true);
 		floppy_revalidate(disk);
+	}
 	return 0;
 }
 
-- 
2.39.2

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Ming Lei]

On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote:
> disk_check_media_change is mostly called from ->open where it makes
> little sense to mark the file system on the device as dead, as we
> are just opening it.  So instead of calling bdev_mark_dead from
> disk_check_media_change move it into the few callers that are not
> in an open instance.  This avoid calling into bdev_mark_dead and
> thus taking s_umount with open_mutex held.

->open() is called when opening bdev every times, and there can be
existed openers, so not sure if it makes little sense here.


Thanks,
Ming

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Christoph Hellwig]

On Wed, Oct 18, 2023 at 11:16:28AM +0800, Ming Lei wrote:
> On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote:
> > disk_check_media_change is mostly called from ->open where it makes
> > little sense to mark the file system on the device as dead, as we
> > are just opening it.  So instead of calling bdev_mark_dead from
> > disk_check_media_change move it into the few callers that are not
> > in an open instance.  This avoid calling into bdev_mark_dead and
> > thus taking s_umount with open_mutex held.
> 
> ->open() is called when opening bdev every times, and there can be
> existed openers, so not sure if it makes little sense here.

Yes. It has to be a non-exclusive open, though, and how is that going
to help us with any general use case?  If we really want to make the
media change notifications any useful we'd better just do the work
straight from the workqueue that polls for it.

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Ming Lei]

On Wed, Oct 18, 2023 at 08:46:46AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 18, 2023 at 11:16:28AM +0800, Ming Lei wrote:
> > On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote:
> > > disk_check_media_change is mostly called from ->open where it makes
> > > little sense to mark the file system on the device as dead, as we
> > > are just opening it.  So instead of calling bdev_mark_dead from
> > > disk_check_media_change move it into the few callers that are not
> > > in an open instance.  This avoid calling into bdev_mark_dead and
> > > thus taking s_umount with open_mutex held.
> > 
> > ->open() is called when opening bdev every times, and there can be
> > existed openers, so not sure if it makes little sense here.
> 
> Yes. It has to be a non-exclusive open, though, and how is that going
> to help us with any general use case?  If we really want to make the
> media change notifications any useful we'd better just do the work
> straight from the workqueue that polls for it.

Given ->mark_dead() is added recently, userspace shouldn't depend on
this behavior, so this patch looks fine:

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Christoph Hellwig]

On Wed, Oct 18, 2023 at 05:15:36PM +0800, Ming Lei wrote:
> > Yes. It has to be a non-exclusive open, though, and how is that going
> > to help us with any general use case?  If we really want to make the
> > media change notifications any useful we'd better just do the work
> > straight from the workqueue that polls for it.
> 
> Given ->mark_dead() is added recently, userspace shouldn't depend on
> this behavior, so this patch looks fine:

While the method is new, disk_check_media_change called
__invalidate_device before, which ended up both taking s_umount and
thrashing the buffer cache under the fs.

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Christian Brauner]

On Tue, Oct 17, 2023 at 08:48:21PM +0200, Christoph Hellwig wrote:
> disk_check_media_change is mostly called from ->open where it makes
> little sense to mark the file system on the device as dead, as we
> are just opening it.  So instead of calling bdev_mark_dead from
> disk_check_media_change move it into the few callers that are not
> in an open instance.  This avoid calling into bdev_mark_dead and
> thus taking s_umount with open_mutex held.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Christoph Hellwig]

I turns out that we'd need bdev_mark_dead generally exported for this.
I don't quite like that, but I don't really see a way around it.
Maybe fix that up in your tree?

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Christian Brauner]

On Thu, Oct 19, 2023 at 07:57:40AM +0200, Christoph Hellwig wrote:
> I turns out that we'd need bdev_mark_dead generally exported for this.
> I don't quite like that, but I don't really see a way around it.
> Maybe fix that up in your tree?

Done.

Re: [PATCH 3/5] block: move bdev_mark_dead out of disk_check_media_change

[Jan Kara]

On Tue 17-10-23 20:48:21, Christoph Hellwig wrote:
> disk_check_media_change is mostly called from ->open where it makes
> little sense to mark the file system on the device as dead, as we
> are just opening it.  So instead of calling bdev_mark_dead from
> disk_check_media_change move it into the few callers that are not
> in an open instance.  This avoid calling into bdev_mark_dead and
> thus taking s_umount with open_mutex held.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nice and looks good to me (with the fixed up export). Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  block/disk-events.c     | 18 +++++++-----------
>  drivers/block/ataflop.c |  4 +++-
>  drivers/block/floppy.c  |  4 +++-
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/block/disk-events.c b/block/disk-events.c
> index 13c3372c465a39..2f697224386aa8 100644
> --- a/block/disk-events.c
> +++ b/block/disk-events.c
> @@ -266,11 +266,8 @@ static unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask)
>   * disk_check_media_change - check if a removable media has been changed
>   * @disk: gendisk to check
>   *
> - * Check whether a removable media has been changed, and attempt to free all
> - * dentries and inodes and invalidates all block device page cache entries in
> - * that case.
> - *
> - * Returns %true if the media has changed, or %false if not.
> + * Returns %true and marks the disk for a partition rescan whether a removable
> + * media has been changed, and %false if the media did not change.
>   */
>  bool disk_check_media_change(struct gendisk *disk)
>  {
> @@ -278,12 +275,11 @@ bool disk_check_media_change(struct gendisk *disk)
>  
>  	events = disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE |
>  				   DISK_EVENT_EJECT_REQUEST);
> -	if (!(events & DISK_EVENT_MEDIA_CHANGE))
> -		return false;
> -
> -	bdev_mark_dead(disk->part0, true);
> -	set_bit(GD_NEED_PART_SCAN, &disk->state);
> -	return true;
> +	if (events & DISK_EVENT_MEDIA_CHANGE) {
> +		set_bit(GD_NEED_PART_SCAN, &disk->state);
> +		return true;
> +	}
> +	return false;
>  }
>  EXPORT_SYMBOL(disk_check_media_change);
>  
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index cd738cab725f39..50949207798d2a 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -1760,8 +1760,10 @@ static int fd_locked_ioctl(struct block_device *bdev, blk_mode_t mode,
>  		/* invalidate the buffer track to force a reread */
>  		BufferDrive = -1;
>  		set_bit(drive, &fake_change);
> -		if (disk_check_media_change(disk))
> +		if (disk_check_media_change(disk)) {
> +			bdev_mark_dead(disk->part0, true);
>  			floppy_revalidate(disk);
> +		}
>  		return 0;
>  	default:
>  		return -EINVAL;
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index ea4eb88a2e45f4..11114a5d9e5c46 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3215,8 +3215,10 @@ static int invalidate_drive(struct gendisk *disk)
>  	/* invalidate the buffer track to force a reread */
>  	set_bit((long)disk->private_data, &fake_change);
>  	process_fd_request();
> -	if (disk_check_media_change(disk))
> +	if (disk_check_media_change(disk)) {
> +		bdev_mark_dead(disk->part0, true);
>  		floppy_revalidate(disk);
> +	}
>  	return 0;
>  }
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 4/5] block: assert that we're not holding open_mutex over blk_report_disk_dead

[Christoph Hellwig]

From: Christian Brauner <brauner@kernel.org>

blk_report_disk_dead() has the following major callers:

(1) del_gendisk()
(2) blk_mark_disk_dead()

Since del_gendisk() acquires disk->open_mutex it's clear that all
callers are assumed to be called without disk->open_mutex held.
In turn, blk_report_disk_dead() is called without disk->open_mutex held
in del_gendisk().

All callers of blk_mark_disk_dead() call it without disk->open_mutex as
well.

Ensure that it is clear that blk_report_disk_dead() is called without
disk->open_mutex on purpose by asserting it and a comment in the code.

Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index 4a16a424f57d4f..c9d06f72c587e8 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -559,6 +559,13 @@ static void blk_report_disk_dead(struct gendisk *disk, bool surprise)
 	struct block_device *bdev;
 	unsigned long idx;
 
+	/*
+	 * On surprise disk removal, bdev_mark_dead() may call into file
+	 * systems below. Make it clear that we're expecting to not hold
+	 * disk->open_mutex.
+	 */
+	lockdep_assert_not_held(&disk->open_mutex);
+
 	rcu_read_lock();
 	xa_for_each(&disk->part_tbl, idx, bdev) {
 		if (!kobject_get_unless_zero(&bdev->bd_device.kobj))
-- 
2.39.2

Re: [PATCH 4/5] block: assert that we're not holding open_mutex over blk_report_disk_dead

[Ming Lei]

On Tue, Oct 17, 2023 at 08:48:22PM +0200, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> blk_report_disk_dead() has the following major callers:
> 
> (1) del_gendisk()
> (2) blk_mark_disk_dead()
> 
> Since del_gendisk() acquires disk->open_mutex it's clear that all
> callers are assumed to be called without disk->open_mutex held.
> In turn, blk_report_disk_dead() is called without disk->open_mutex held
> in del_gendisk().
> 
> All callers of blk_mark_disk_dead() call it without disk->open_mutex as
> well.
> 
> Ensure that it is clear that blk_report_disk_dead() is called without
> disk->open_mutex on purpose by asserting it and a comment in the code.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/genhd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 4a16a424f57d4f..c9d06f72c587e8 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -559,6 +559,13 @@ static void blk_report_disk_dead(struct gendisk *disk, bool surprise)
>  	struct block_device *bdev;
>  	unsigned long idx;
>  
> +	/*
> +	 * On surprise disk removal, bdev_mark_dead() may call into file
> +	 * systems below. Make it clear that we're expecting to not hold
> +	 * disk->open_mutex.
> +	 */
> +	lockdep_assert_not_held(&disk->open_mutex);
> +
>  	rcu_read_lock();
>  	xa_for_each(&disk->part_tbl, idx, bdev) {
>  		if (!kobject_get_unless_zero(&bdev->bd_device.kobj))

Reviewed-by: Ming Lei <ming.lei@redhat.com>


thanks, 
Ming

Re: [PATCH 4/5] block: assert that we're not holding open_mutex over blk_report_disk_dead

[Jan Kara]

On Tue 17-10-23 20:48:22, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> blk_report_disk_dead() has the following major callers:
> 
> (1) del_gendisk()
> (2) blk_mark_disk_dead()
> 
> Since del_gendisk() acquires disk->open_mutex it's clear that all
> callers are assumed to be called without disk->open_mutex held.
> In turn, blk_report_disk_dead() is called without disk->open_mutex held
> in del_gendisk().
> 
> All callers of blk_mark_disk_dead() call it without disk->open_mutex as
> well.
> 
> Ensure that it is clear that blk_report_disk_dead() is called without
> disk->open_mutex on purpose by asserting it and a comment in the code.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Sure. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

BTW, checking the callers I suspect that we might eventually hit some
locking issues with NVME and its ctrl->namespace_sem which is held while
calling blk_mark_disk_dead(). But I guess we'll deal with that once we see
the problem is real.

								Honza

> ---
>  block/genhd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 4a16a424f57d4f..c9d06f72c587e8 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -559,6 +559,13 @@ static void blk_report_disk_dead(struct gendisk *disk, bool surprise)
>  	struct block_device *bdev;
>  	unsigned long idx;
>  
> +	/*
> +	 * On surprise disk removal, bdev_mark_dead() may call into file
> +	 * systems below. Make it clear that we're expecting to not hold
> +	 * disk->open_mutex.
> +	 */
> +	lockdep_assert_not_held(&disk->open_mutex);
> +
>  	rcu_read_lock();
>  	xa_for_each(&disk->part_tbl, idx, bdev) {
>  		if (!kobject_get_unless_zero(&bdev->bd_device.kobj))
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 5/5] fs: assert that open_mutex isn't held over holder ops

[Christoph Hellwig]

From: Christian Brauner <brauner@kernel.org>

With recent block level changes we should never be in a situation where
we hold disk->open_mutex when calling into these helpers. So assert that
in the code.

Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index 26b96191e9b3ca..ce54cfcecaa156 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1443,6 +1443,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
 
 	/* bd_holder_lock ensures that the sb isn't freed */
 	lockdep_assert_held(&bdev->bd_holder_lock);
+	lockdep_assert_not_held(&bdev->bd_disk->open_mutex);
 
 	if (!super_lock_shared_active(sb))
 		return;
@@ -1462,6 +1463,7 @@ static void fs_bdev_sync(struct block_device *bdev)
 	struct super_block *sb = bdev->bd_holder;
 
 	lockdep_assert_held(&bdev->bd_holder_lock);
+	lockdep_assert_not_held(&bdev->bd_disk->open_mutex);
 
 	if (!super_lock_shared_active(sb))
 		return;
-- 
2.39.2

Re: [PATCH 5/5] fs: assert that open_mutex isn't held over holder ops

[Ming Lei]

On Tue, Oct 17, 2023 at 08:48:23PM +0200, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> With recent block level changes we should never be in a situation where
> we hold disk->open_mutex when calling into these helpers. So assert that
> in the code.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

thanks,
Ming

Re: [PATCH 5/5] fs: assert that open_mutex isn't held over holder ops

[Jan Kara]

On Tue 17-10-23 20:48:23, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> With recent block level changes we should never be in a situation where
> we hold disk->open_mutex when calling into these helpers. So assert that
> in the code.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 26b96191e9b3ca..ce54cfcecaa156 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1443,6 +1443,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>  
>  	/* bd_holder_lock ensures that the sb isn't freed */
>  	lockdep_assert_held(&bdev->bd_holder_lock);
> +	lockdep_assert_not_held(&bdev->bd_disk->open_mutex);
>  
>  	if (!super_lock_shared_active(sb))
>  		return;
> @@ -1462,6 +1463,7 @@ static void fs_bdev_sync(struct block_device *bdev)
>  	struct super_block *sb = bdev->bd_holder;
>  
>  	lockdep_assert_held(&bdev->bd_holder_lock);
> +	lockdep_assert_not_held(&bdev->bd_disk->open_mutex);
>  
>  	if (!super_lock_shared_active(sb))
>  		return;
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: don't take s_umount under open_mutex

[Christian Brauner]

On Tue, 17 Oct 2023 20:48:18 +0200, Christoph Hellwig wrote:
> Christian has been pestering Jan and me a bit about finally fixing
> all the pre-existing mostly theoretical cases of s_umount taken under
> open_mutex.  This series, which is mostly from him with some help from
> me should get us to that goal by replacing bdev_mark_dead calls that
> can't ever reach a file system holder to call into with simple bdev
> page invalidation.
> 
> [...]

I've applied this so it ends up in -next now.
@Jens, let me know if you have objections.

---

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[1/5] block: simplify bdev_del_partition()
      https://git.kernel.org/vfs/vfs/c/b0df741ed69d
[2/5] block: WARN_ON_ONCE() when we remove active partitions
      https://git.kernel.org/vfs/vfs/c/2ff3adfb95a3
[3/5] block: move bdev_mark_dead out of disk_check_media_change
      https://git.kernel.org/vfs/vfs/c/6d4367bc04fd
[4/5] block: assert that we're not holding open_mutex over blk_report_disk_dead
      https://git.kernel.org/vfs/vfs/c/7addcb222703
[5/5] fs: assert that open_mutex isn't held over holder ops
      https://git.kernel.org/vfs/vfs/c/43ab05549df4

Re: don't take s_umount under open_mutex

[Jens Axboe]

On 10/19/23 3:35 AM, Christian Brauner wrote:
> On Tue, 17 Oct 2023 20:48:18 +0200, Christoph Hellwig wrote:
>> Christian has been pestering Jan and me a bit about finally fixing
>> all the pre-existing mostly theoretical cases of s_umount taken under
>> open_mutex.  This series, which is mostly from him with some help from
>> me should get us to that goal by replacing bdev_mark_dead calls that
>> can't ever reach a file system holder to call into with simple bdev
>> page invalidation.
>>
>> [...]
> 
> I've applied this so it ends up in -next now.
> @Jens, let me know if you have objections.

Looks fine to me, please add:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

for the series.

-- 
Jens Axboe