Only call truncate_bdev_range() if the fallocate mode is supported. This fixes a bug where data in the pagecache could be invalidated if the fallocate() was called on the block device with an invalid mode.
Fixes: 25f4c41415e5 ("block: implement (some of) fallocate for block devices") Cc: stable@vger.kernel.org Reported-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Sarthak Kukreti sarthakkukreti@chromium.org Reviewed-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Mike Snitzer snitzer@kernel.org --- block/fops.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/block/fops.c b/block/fops.c index acff3d5d22d4..73e42742543f 100644 --- a/block/fops.c +++ b/block/fops.c @@ -772,24 +772,35 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
filemap_invalidate_lock(inode->i_mapping);
- /* Invalidate the page cache, including dirty pages. */ - error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end); - if (error) - goto fail; - + /* + * Invalidate the page cache, including dirty pages, for valid + * de-allocate mode calls to fallocate(). + */ switch (mode) { case FALLOC_FL_ZERO_RANGE: case FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE: + error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end); + if (error) + goto fail; + error = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, len >> SECTOR_SHIFT, GFP_KERNEL, BLKDEV_ZERO_NOUNMAP); break; case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE: + error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end); + if (error) + goto fail; + error = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, len >> SECTOR_SHIFT, GFP_KERNEL, BLKDEV_ZERO_NOFALLBACK); break; case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE: + error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end); + if (error) + goto fail; + error = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT, len >> SECTOR_SHIFT, GFP_KERNEL); break;
On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
Only call truncate_bdev_range() if the fallocate mode is supported. This fixes a bug where data in the pagecache could be invalidated if the fallocate() was called on the block device with an invalid mode.
Fix looks fine, but would be nicer if we didn't have to duplicate the truncate_bdev_range() in each switch clause. Can we check this upfront instead?
Also, please wrap commit messages at 72-74 chars.
On Wed, Oct 11 2023 at 4:20P -0400, Jens Axboe axboe@kernel.dk wrote:
On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
Only call truncate_bdev_range() if the fallocate mode is supported. This fixes a bug where data in the pagecache could be invalidated if the fallocate() was called on the block device with an invalid mode.
Fix looks fine, but would be nicer if we didn't have to duplicate the truncate_bdev_range() in each switch clause. Can we check this upfront instead?
No, if you look at the function (rather than just the patch in isolation) we need to make the call for each case rather than collapse to a single call at the front (that's the reason for this fix, because otherwise the default: error case will invalidate the page cache too).
Just so you're aware, I also had this feedback that shaped the patch a bit back in April: https://listman.redhat.com/archives/dm-devel/2023-April/053986.html
Also, please wrap commit messages at 72-74 chars.
Not seeing where the header should be wrapped. You referring to the Fixes: line? I've never seen those wrapped.
Mike
On 10/11/23 2:50 PM, Mike Snitzer wrote:
On Wed, Oct 11 2023 at 4:20P -0400, Jens Axboe axboe@kernel.dk wrote:
On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
Only call truncate_bdev_range() if the fallocate mode is supported. This fixes a bug where data in the pagecache could be invalidated if the fallocate() was called on the block device with an invalid mode.
Fix looks fine, but would be nicer if we didn't have to duplicate the truncate_bdev_range() in each switch clause. Can we check this upfront instead?
No, if you look at the function (rather than just the patch in isolation) we need to make the call for each case rather than collapse to a single call at the front (that's the reason for this fix, because otherwise the default: error case will invalidate the page cache too).
Yes that part is clear, but it might look cleaner to check a valid mask first rather than have 3 duplicate calls.
Just so you're aware, I also had this feedback that shaped the patch a bit back in April: https://listman.redhat.com/archives/dm-devel/2023-April/053986.html
Also, please wrap commit messages at 72-74 chars.
Not seeing where the header should be wrapped. You referring to the Fixes: line? I've never seen those wrapped.
I'm referring to the commit message itself.
On Wed, Oct 11 2023 at 4:53P -0400, Jens Axboe axboe@kernel.dk wrote:
On 10/11/23 2:50 PM, Mike Snitzer wrote:
On Wed, Oct 11 2023 at 4:20P -0400, Jens Axboe axboe@kernel.dk wrote:
On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
Only call truncate_bdev_range() if the fallocate mode is supported. This fixes a bug where data in the pagecache could be invalidated if the fallocate() was called on the block device with an invalid mode.
Fix looks fine, but would be nicer if we didn't have to duplicate the truncate_bdev_range() in each switch clause. Can we check this upfront instead?
No, if you look at the function (rather than just the patch in isolation) we need to make the call for each case rather than collapse to a single call at the front (that's the reason for this fix, because otherwise the default: error case will invalidate the page cache too).
Yes that part is clear, but it might look cleaner to check a valid mask first rather than have 3 duplicate calls.
OK.
Just so you're aware, I also had this feedback that shaped the patch a bit back in April: https://listman.redhat.com/archives/dm-devel/2023-April/053986.html
Also, please wrap commit messages at 72-74 chars.
Not seeing where the header should be wrapped. You referring to the Fixes: line? I've never seen those wrapped.
I'm referring to the commit message itself.
Ah, you'd like lines extended because they are too short.
On 10/11/23 3:08 PM, Mike Snitzer wrote:
Also, please wrap commit messages at 72-74 chars.
Not seeing where the header should be wrapped. You referring to the Fixes: line? I've never seen those wrapped.
I'm referring to the commit message itself.
Ah, you'd like lines extended because they are too short.
Exactly, it's way too short.
On 10/11/23 2:20 PM, Jens Axboe wrote:
On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
Only call truncate_bdev_range() if the fallocate mode is supported. This fixes a bug where data in the pagecache could be invalidated if the fallocate() was called on the block device with an invalid mode.
Fix looks fine, but would be nicer if we didn't have to duplicate the truncate_bdev_range() in each switch clause. Can we check this upfront instead?
Don't see a good way to do it on my end, so let's just go with what is there now. I applied it with the commit message reformatted.
On Wed, 11 Oct 2023 13:12:30 -0700, Sarthak Kukreti wrote:
Only call truncate_bdev_range() if the fallocate mode is supported. This fixes a bug where data in the pagecache could be invalidated if the fallocate() was called on the block device with an invalid mode.
Applied, thanks!
[1/1] block: Don't invalidate pagecache for invalid falloc modes commit: 1364a3c391aedfeb32aa025303ead3d7c91cdf9d
Best regards,
linux-stable-mirror@lists.linaro.org
-
Jens Axboe -
Mike Snitzer -
Sarthak Kukreti