[PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[Darrick J. Wong]

Hi all,

Here are a handful of bugfixes for 6.15.  The first patch fixes a race
between set_blocksize and block device pagecache manipulation; the rest
removes XFS' usage of set_blocksize since it's unnecessary.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

With a bit of luck, this should all go splendidly.
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=bdev-fixes-6.15
---
Commits in this patchset:
 * block: fix race between set_blocksize and read paths
 * block: hoist block size validation code to a separate function
 * xfs: stop using set_blocksize
---
 include/linux/blkdev.h |    1 +
 block/bdev.c           |   50 ++++++++++++++++++++++++++++++++++++++++++------
 block/blk-zoned.c      |    5 ++++-
 block/fops.c           |   16 +++++++++++++++
 block/ioctl.c          |    6 ++++++
 fs/xfs/xfs_buf.c       |   15 +++++++++++---
 6 files changed, 82 insertions(+), 11 deletions(-)

[PATCH 1/3] block: fix race between set_blocksize and read paths

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

With the new large sector size support, it's now the case that
set_blocksize can change i_blksize and the folio order in a manner that
conflicts with a concurrent reader and causes a kernel crash.

Specifically, let's say that udev-worker calls libblkid to detect the
labels on a block device.  The read call can create an order-0 folio to
read the first 4096 bytes from the disk.  But then udev is preempted.

Next, someone tries to mount an 8k-sectorsize filesystem from the same
block device.  The filesystem calls set_blksize, which sets i_blksize to
8192 and the minimum folio order to 1.

Now udev resumes, still holding the order-0 folio it allocated.  It then
tries to schedule a read bio and do_mpage_readahead tries to create
bufferheads for the folio.  Unfortunately, blocks_per_folio == 0 because
the page size is 4096 but the blocksize is 8192 so no bufferheads are
attached and the bh walk never sets bdev.  We then submit the bio with a
NULL block device and crash.

Therefore, truncate the page cache after flushing but before updating
i_blksize.  However, that's not enough -- we also need to lock out file
IO and page faults during the update.  Take both the i_rwsem and the
invalidate_lock in exclusive mode for invalidations, and in shared mode
for read/write operations.

I don't know if this is the correct fix, but xfs/259 found it.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/bdev.c      |   17 +++++++++++++++++
 block/blk-zoned.c |    5 ++++-
 block/fops.c      |   16 ++++++++++++++++
 block/ioctl.c     |    6 ++++++
 4 files changed, 43 insertions(+), 1 deletion(-)


diff --git a/block/bdev.c b/block/bdev.c
index 6a2d08166e50c7..24984ec13e7cb2 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -169,11 +169,28 @@ int set_blocksize(struct file *file, int size)
 
 	/* Don't change the size if it is same as current */
 	if (inode->i_blkbits != blksize_bits(size)) {
+		/*
+		 * Flush and truncate the pagecache before we reconfigure the
+		 * mapping geometry because folio sizes are variable now.  If a
+		 * reader has already allocated a folio whose size is smaller
+		 * than the new min_order but invokes readahead after the new
+		 * min_order becomes visible, readahead will think there are
+		 * "zero" blocks per folio and crash.  Take the inode and
+		 * invalidation locks to avoid racing with
+		 * read/write/fallocate.
+		 */
+		inode_lock(inode);
+		filemap_invalidate_lock(inode->i_mapping);
+
 		sync_blockdev(bdev);
+		kill_bdev(bdev);
+
 		inode->i_blkbits = blksize_bits(size);
 		mapping_set_folio_order_range(inode->i_mapping,
 				get_order(size), get_order(size));
 		kill_bdev(bdev);
+		filemap_invalidate_unlock(inode->i_mapping);
+		inode_unlock(inode);
 	}
 	return 0;
 }
diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 0c77244a35c92e..8f15d1aa6eb89a 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -343,6 +343,7 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode,
 		op = REQ_OP_ZONE_RESET;
 
 		/* Invalidate the page cache, including dirty pages. */
+		inode_lock(bdev->bd_mapping->host);
 		filemap_invalidate_lock(bdev->bd_mapping);
 		ret = blkdev_truncate_zone_range(bdev, mode, &zrange);
 		if (ret)
@@ -364,8 +365,10 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, blk_mode_t mode,
 	ret = blkdev_zone_mgmt(bdev, op, zrange.sector, zrange.nr_sectors);
 
 fail:
-	if (cmd == BLKRESETZONE)
+	if (cmd == BLKRESETZONE) {
 		filemap_invalidate_unlock(bdev->bd_mapping);
+		inode_unlock(bdev->bd_mapping->host);
+	}
 
 	return ret;
 }
diff --git a/block/fops.c b/block/fops.c
index be9f1dbea9ce0a..e221fdcaa8aaf8 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -746,7 +746,14 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 			ret = direct_write_fallback(iocb, from, ret,
 					blkdev_buffered_write(iocb, from));
 	} else {
+		/*
+		 * Take i_rwsem and invalidate_lock to avoid racing with
+		 * set_blocksize changing i_blkbits/folio order and punching
+		 * out the pagecache.
+		 */
+		inode_lock_shared(bd_inode);
 		ret = blkdev_buffered_write(iocb, from);
+		inode_unlock_shared(bd_inode);
 	}
 
 	if (ret > 0)
@@ -757,6 +764,7 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
+	struct inode *bd_inode = bdev_file_inode(iocb->ki_filp);
 	struct block_device *bdev = I_BDEV(iocb->ki_filp->f_mapping->host);
 	loff_t size = bdev_nr_bytes(bdev);
 	loff_t pos = iocb->ki_pos;
@@ -793,7 +801,13 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 			goto reexpand;
 	}
 
+	/*
+	 * Take i_rwsem and invalidate_lock to avoid racing with set_blocksize
+	 * changing i_blkbits/folio order and punching out the pagecache.
+	 */
+	inode_lock_shared(bd_inode);
 	ret = filemap_read(iocb, to, ret);
+	inode_unlock_shared(bd_inode);
 
 reexpand:
 	if (unlikely(shorted))
@@ -836,6 +850,7 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
 	if ((start | len) & (bdev_logical_block_size(bdev) - 1))
 		return -EINVAL;
 
+	inode_lock(inode);
 	filemap_invalidate_lock(inode->i_mapping);
 
 	/*
@@ -868,6 +883,7 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
 
  fail:
 	filemap_invalidate_unlock(inode->i_mapping);
+	inode_unlock(inode);
 	return error;
 }
 
diff --git a/block/ioctl.c b/block/ioctl.c
index faa40f383e2736..e472cc1030c60c 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -142,6 +142,7 @@ static int blk_ioctl_discard(struct block_device *bdev, blk_mode_t mode,
 	if (err)
 		return err;
 
+	inode_lock(bdev->bd_mapping->host);
 	filemap_invalidate_lock(bdev->bd_mapping);
 	err = truncate_bdev_range(bdev, mode, start, start + len - 1);
 	if (err)
@@ -174,6 +175,7 @@ static int blk_ioctl_discard(struct block_device *bdev, blk_mode_t mode,
 	blk_finish_plug(&plug);
 fail:
 	filemap_invalidate_unlock(bdev->bd_mapping);
+	inode_unlock(bdev->bd_mapping->host);
 	return err;
 }
 
@@ -199,12 +201,14 @@ static int blk_ioctl_secure_erase(struct block_device *bdev, blk_mode_t mode,
 	    end > bdev_nr_bytes(bdev))
 		return -EINVAL;
 
+	inode_lock(bdev->bd_mapping->host);
 	filemap_invalidate_lock(bdev->bd_mapping);
 	err = truncate_bdev_range(bdev, mode, start, end - 1);
 	if (!err)
 		err = blkdev_issue_secure_erase(bdev, start >> 9, len >> 9,
 						GFP_KERNEL);
 	filemap_invalidate_unlock(bdev->bd_mapping);
+	inode_unlock(bdev->bd_mapping->host);
 	return err;
 }
 
@@ -236,6 +240,7 @@ static int blk_ioctl_zeroout(struct block_device *bdev, blk_mode_t mode,
 		return -EINVAL;
 
 	/* Invalidate the page cache, including dirty pages */
+	inode_lock(bdev->bd_mapping->host);
 	filemap_invalidate_lock(bdev->bd_mapping);
 	err = truncate_bdev_range(bdev, mode, start, end);
 	if (err)
@@ -246,6 +251,7 @@ static int blk_ioctl_zeroout(struct block_device *bdev, blk_mode_t mode,
 
 fail:
 	filemap_invalidate_unlock(bdev->bd_mapping);
+	inode_unlock(bdev->bd_mapping->host);
 	return err;
 }
 

[PATCH 2/3] block: hoist block size validation code to a separate function

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Hoist the block size validation code to bdev_validate_blocksize so that
we can call it from filesystems that don't care about the bdev pagecache
manipulations of set_blocksize.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 include/linux/blkdev.h |    1 +
 block/bdev.c           |   33 +++++++++++++++++++++++++++------
 2 files changed, 28 insertions(+), 6 deletions(-)


diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 294bbae415aa09..462e23a1e8b261 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1618,6 +1618,7 @@ static inline void bio_end_io_acct(struct bio *bio, unsigned long start_time)
 	return bio_end_io_acct_remapped(bio, start_time, bio->bi_bdev);
 }
 
+int bdev_validate_blocksize(struct block_device *bdev, int block_size);
 int set_blocksize(struct file *file, int size);
 
 int lookup_bdev(const char *pathname, dev_t *dev);
diff --git a/block/bdev.c b/block/bdev.c
index 24984ec13e7cb2..1588f96e4f0a35 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -152,17 +152,38 @@ static void set_init_blocksize(struct block_device *bdev)
 				    get_order(bsize), get_order(bsize));
 }
 
+/**
+ * bdev_validate_blocksize - check that this block size is acceptable
+ * @bdev:	blockdevice to check
+ * @block_size:	block size to check
+ *
+ * For block device users that do not use buffer heads or the block device
+ * page cache, make sure that this block size can be used with the device.
+ *
+ * Return: On success zero is returned, negative error code on failure.
+ */
+int bdev_validate_blocksize(struct block_device *bdev, int block_size)
+{
+	if (blk_validate_block_size(block_size))
+		return -EINVAL;
+
+	/* Size cannot be smaller than the size supported by the device */
+	if (block_size < bdev_logical_block_size(bdev))
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bdev_validate_blocksize);
+
 int set_blocksize(struct file *file, int size)
 {
 	struct inode *inode = file->f_mapping->host;
 	struct block_device *bdev = I_BDEV(inode);
+	int ret;
 
-	if (blk_validate_block_size(size))
-		return -EINVAL;
-
-	/* Size cannot be smaller than the size supported by the device */
-	if (size < bdev_logical_block_size(bdev))
-		return -EINVAL;
+	ret = bdev_validate_blocksize(bdev, size);
+	if (ret)
+		return ret;
 
 	if (!file->private_data)
 		return -EINVAL;

[PATCH 3/3] xfs: stop using set_blocksize

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

XFS has its own buffer cache for metadata that uses submit_bio, which
means that it no longer uses the block device pagecache for anything.
Create a more lightweight helper that runs the blocksize checks and
flushes dirty data and use that instead.  No more truncating the
pagecache because XFS does not use it or care about it.

Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_buf.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 1a2b3f06fa717e..5ae77ffdc947b1 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1719,18 +1719,25 @@ xfs_setsize_buftarg(
 	struct xfs_buftarg	*btp,
 	unsigned int		sectorsize)
 {
+	int			error;
+
 	/* Set up metadata sector size info */
 	btp->bt_meta_sectorsize = sectorsize;
 	btp->bt_meta_sectormask = sectorsize - 1;
 
-	if (set_blocksize(btp->bt_bdev_file, sectorsize)) {
+	error = bdev_validate_blocksize(btp->bt_bdev, sectorsize);
+	if (error) {
 		xfs_warn(btp->bt_mount,
-			"Cannot set_blocksize to %u on device %pg",
-			sectorsize, btp->bt_bdev);
+			"Cannot use blocksize %u on device %pg, err %d",
+			sectorsize, btp->bt_bdev, error);
 		return -EINVAL;
 	}
 
-	return 0;
+	/*
+	 * Flush the block device pagecache so our bios see anything dirtied
+	 * before mount.
+	 */
+	return sync_blockdev(btp->bt_bdev);
 }
 
 int

Re: [PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[Jens Axboe]

On 4/21/25 11:18 AM, Darrick J. Wong wrote:
> Hi all,
> 
> Here are a handful of bugfixes for 6.15.  The first patch fixes a race
> between set_blocksize and block device pagecache manipulation; the rest
> removes XFS' usage of set_blocksize since it's unnecessary.
> 
> If you're going to start using this code, I strongly recommend pulling
> from my git trees, which are linked below.
> 
> With a bit of luck, this should all go splendidly.
> Comments and questions are, as always, welcome.

block changes look good to me - I'll tentatively queue those up.

-- 
Jens Axboe

Re: [PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[Jens Axboe]

On 4/21/25 2:24 PM, Jens Axboe wrote:
> On 4/21/25 11:18 AM, Darrick J. Wong wrote:
>> Hi all,
>>
>> Here are a handful of bugfixes for 6.15.  The first patch fixes a race
>> between set_blocksize and block device pagecache manipulation; the rest
>> removes XFS' usage of set_blocksize since it's unnecessary.
>>
>> If you're going to start using this code, I strongly recommend pulling
>> from my git trees, which are linked below.
>>
>> With a bit of luck, this should all go splendidly.
>> Comments and questions are, as always, welcome.
> 
> block changes look good to me - I'll tentatively queue those up.

Hmm looks like it's built on top of other changes in your branch,
doesn't apply cleanly.

-- 
Jens Axboe

Re: [PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[Darrick J. Wong]

On Mon, Apr 21, 2025 at 02:26:54PM -0600, Jens Axboe wrote:
> On 4/21/25 2:24 PM, Jens Axboe wrote:
> > On 4/21/25 11:18 AM, Darrick J. Wong wrote:
> >> Hi all,
> >>
> >> Here are a handful of bugfixes for 6.15.  The first patch fixes a race
> >> between set_blocksize and block device pagecache manipulation; the rest
> >> removes XFS' usage of set_blocksize since it's unnecessary.
> >>
> >> If you're going to start using this code, I strongly recommend pulling
> >> from my git trees, which are linked below.
> >>
> >> With a bit of luck, this should all go splendidly.
> >> Comments and questions are, as always, welcome.
> > 
> > block changes look good to me - I'll tentatively queue those up.
> 
> Hmm looks like it's built on top of other changes in your branch,
> doesn't apply cleanly.

Yeah, I'm still waiting for hch (or anyone) to RVB patches 2 and 3.

--D

> -- 
> Jens Axboe

Re: [PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[Jens Axboe]

On 4/21/25 2:51 PM, Darrick J. Wong wrote:
> On Mon, Apr 21, 2025 at 02:26:54PM -0600, Jens Axboe wrote:
>> On 4/21/25 2:24 PM, Jens Axboe wrote:
>>> On 4/21/25 11:18 AM, Darrick J. Wong wrote:
>>>> Hi all,
>>>>
>>>> Here are a handful of bugfixes for 6.15.  The first patch fixes a race
>>>> between set_blocksize and block device pagecache manipulation; the rest
>>>> removes XFS' usage of set_blocksize since it's unnecessary.
>>>>
>>>> If you're going to start using this code, I strongly recommend pulling
>>>> from my git trees, which are linked below.
>>>>
>>>> With a bit of luck, this should all go splendidly.
>>>> Comments and questions are, as always, welcome.
>>>
>>> block changes look good to me - I'll tentatively queue those up.
>>
>> Hmm looks like it's built on top of other changes in your branch,
>> doesn't apply cleanly.
> 
> Yeah, I'm still waiting for hch (or anyone) to RVB patches 2 and 3.

Maybe I wasn't 100% clear, but what I mean is that patches 1 and 2 don't
apply to the upstream kernel, as they are sitting on top of other
patches that block block/bdev.c in your tree. So even if acked, they
can't go in as-is. Well they can, I'd just have to hand apply them.
Which isn't the end of the world, but the dependency wasn't clear (to
me, at least) in the sent out patches.

-- 
Jens Axboe

Re: [PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[Darrick J. Wong]

On Mon, Apr 21, 2025 at 02:53:42PM -0600, Jens Axboe wrote:
> On 4/21/25 2:51 PM, Darrick J. Wong wrote:
> > On Mon, Apr 21, 2025 at 02:26:54PM -0600, Jens Axboe wrote:
> >> On 4/21/25 2:24 PM, Jens Axboe wrote:
> >>> On 4/21/25 11:18 AM, Darrick J. Wong wrote:
> >>>> Hi all,
> >>>>
> >>>> Here are a handful of bugfixes for 6.15.  The first patch fixes a race
> >>>> between set_blocksize and block device pagecache manipulation; the rest
> >>>> removes XFS' usage of set_blocksize since it's unnecessary.
> >>>>
> >>>> If you're going to start using this code, I strongly recommend pulling
> >>>> from my git trees, which are linked below.
> >>>>
> >>>> With a bit of luck, this should all go splendidly.
> >>>> Comments and questions are, as always, welcome.
> >>>
> >>> block changes look good to me - I'll tentatively queue those up.
> >>
> >> Hmm looks like it's built on top of other changes in your branch,
> >> doesn't apply cleanly.
> > 
> > Yeah, I'm still waiting for hch (or anyone) to RVB patches 2 and 3.
> 
> Maybe I wasn't 100% clear, but what I mean is that patches 1 and 2 don't
> apply to the upstream kernel, as they are sitting on top of other
> patches that block block/bdev.c in your tree. So even if acked, they
> can't go in as-is. Well they can, I'd just have to hand apply them.
> Which isn't the end of the world, but the dependency wasn't clear (to
> me, at least) in the sent out patches.

Oh!  Silly me, I forgot that there were debug patches.  Will attach
Luis' review tags, rebase, and resend.  Sorry about that. :/

--D

> -- 
> Jens Axboe
> 

Re: [PATCHSET V2] block/xfs: bdev page cache bug fixes for 6.15

[Luis Chamberlain]

On Mon, Apr 21, 2025 at 10:18:45AM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> Here are a handful of bugfixes for 6.15.  The first patch fixes a race
> between set_blocksize and block device pagecache manipulation; the rest
> removes XFS' usage of set_blocksize since it's unnecessary.
> 
> If you're going to start using this code, I strongly recommend pulling
> from my git trees, which are linked below.
> 
> With a bit of luck, this should all go splendidly.
> Comments and questions are, as always, welcome.

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis