| Message ID | 20230420004850.297045-2-sarthakkukreti@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/5] block: Don't invalidate pagecache for invalid falloc modes | expand |
On Wed, Apr 19, 2023 at 05:48:46PM -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. > > Fixes: 25f4c41415e5 ("block: implement (some of) fallocate for block devices") > Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org> > --- > block/fops.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/block/fops.c b/block/fops.c > index d2e6be4e3d1c..2fd7e8b9ab48 100644 > --- a/block/fops.c > +++ b/block/fops.c > @@ -648,25 +648,27 @@ 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->f_mode, 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 = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, > + error = truncate_bdev_range(bdev, file->f_mode, start, end) || > + blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, I'm pretty sure we're supposed to preserve the error codes returned by both functions. error = truncate_bdev_range(...); if (!error) error = blkdev_issue_zeroout(...); --D > len >> SECTOR_SHIFT, GFP_KERNEL, > BLKDEV_ZERO_NOUNMAP); > break; > case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE: > - error = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, > + error = truncate_bdev_range(bdev, file->f_mode, start, end) || > + 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 = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT, > + error = truncate_bdev_range(bdev, file->f_mode, start, end) || > + blkdev_issue_discard(bdev, start >> SECTOR_SHIFT, > len >> SECTOR_SHIFT, GFP_KERNEL); > break; > default: > -- > 2.40.0.634.g4ca3ef3211-goog >
Sorry I tried to be too concise :) Updated with a fixed up patch! Best Sarhtak On Wed, Apr 19, 2023 at 6:22 PM Darrick J. Wong <djwong@kernel.org> wrote: > > On Wed, Apr 19, 2023 at 05:48:46PM -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. > > > > Fixes: 25f4c41415e5 ("block: implement (some of) fallocate for block devices") > > Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org> > > --- > > block/fops.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/block/fops.c b/block/fops.c > > index d2e6be4e3d1c..2fd7e8b9ab48 100644 > > --- a/block/fops.c > > +++ b/block/fops.c > > @@ -648,25 +648,27 @@ 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->f_mode, 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 = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, > > + error = truncate_bdev_range(bdev, file->f_mode, start, end) || > > + blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, > > I'm pretty sure we're supposed to preserve the error codes returned by > both functions. > > error = truncate_bdev_range(...); > if (!error) > error = blkdev_issue_zeroout(...); > > --D > > > len >> SECTOR_SHIFT, GFP_KERNEL, > > BLKDEV_ZERO_NOUNMAP); > > break; > > case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE: > > - error = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, > > + error = truncate_bdev_range(bdev, file->f_mode, start, end) || > > + 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 = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT, > > + error = truncate_bdev_range(bdev, file->f_mode, start, end) || > > + blkdev_issue_discard(bdev, start >> SECTOR_SHIFT, > > len >> SECTOR_SHIFT, GFP_KERNEL); > > break; > > default: > > -- > > 2.40.0.634.g4ca3ef3211-goog > >
Hi Sarthak,
kernel test robot noticed the following build warnings:
[auto build test WARNING on device-mapper-dm/for-next]
[also build test WARNING on linus/master v6.3 next-20230421]
[cannot apply to axboe-block/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sarthak-Kukreti/block-Introduce-provisioning-primitives/20230420-095025
base: https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git for-next
patch link: https://lore.kernel.org/r/20230420004850.297045-2-sarthakkukreti%40chromium.org
patch subject: [PATCH v5 1/5] block: Don't invalidate pagecache for invalid falloc modes
config: hexagon-randconfig-r006-20230424 (https://download.01.org/0day-ci/archive/20230424/202304242302.5zYRfUub-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 437b7602e4a998220871de78afcb020b9c14a661)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/8bd0744b438be1722c5f8c1fe077e9dcef0e81b7
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Sarthak-Kukreti/block-Introduce-provisioning-primitives/20230420-095025
git checkout 8bd0744b438be1722c5f8c1fe077e9dcef0e81b7
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304242302.5zYRfUub-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from block/fops.c:9:
In file included from include/linux/blkdev.h:9:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from block/fops.c:9:
In file included from include/linux/blkdev.h:9:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from block/fops.c:9:
In file included from include/linux/blkdev.h:9:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> block/fops.c:678:2: warning: unused label 'fail' [-Wunused-label]
fail:
^~~~~
7 warnings generated.
vim +/fail +678 block/fops.c
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 613
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 614 #define BLKDEV_FALLOC_FL_SUPPORTED \
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 615 (FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | \
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 616 FALLOC_FL_ZERO_RANGE | FALLOC_FL_NO_HIDE_STALE)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 617
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 618 static long blkdev_fallocate(struct file *file, int mode, loff_t start,
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 619 loff_t len)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 620 {
f278eb3d8178f9 Ming Lei 2021-09-23 621 struct inode *inode = bdev_file_inode(file);
f278eb3d8178f9 Ming Lei 2021-09-23 622 struct block_device *bdev = I_BDEV(inode);
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 623 loff_t end = start + len - 1;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 624 loff_t isize;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 625 int error;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 626
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 627 /* Fail if we don't recognize the flags. */
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 628 if (mode & ~BLKDEV_FALLOC_FL_SUPPORTED)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 629 return -EOPNOTSUPP;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 630
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 631 /* Don't go off the end of the device. */
2a93ad8fcb377b Christoph Hellwig 2021-10-18 632 isize = bdev_nr_bytes(bdev);
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 633 if (start >= isize)
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 634 return -EINVAL;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 635 if (end >= isize) {
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 636 if (mode & FALLOC_FL_KEEP_SIZE) {
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 637 len = isize - start;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 638 end = start + len - 1;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 639 } else
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 640 return -EINVAL;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 641 }
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 642
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 643 /*
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 644 * Don't allow IO that isn't aligned to logical block size.
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 645 */
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 646 if ((start | len) & (bdev_logical_block_size(bdev) - 1))
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 647 return -EINVAL;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 648
f278eb3d8178f9 Ming Lei 2021-09-23 649 filemap_invalidate_lock(inode->i_mapping);
f278eb3d8178f9 Ming Lei 2021-09-23 650
8bd0744b438be1 Sarthak Kukreti 2023-04-19 651 /*
8bd0744b438be1 Sarthak Kukreti 2023-04-19 652 * Invalidate the page cache, including dirty pages, for valid
8bd0744b438be1 Sarthak Kukreti 2023-04-19 653 * de-allocate mode calls to fallocate().
8bd0744b438be1 Sarthak Kukreti 2023-04-19 654 */
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 655 switch (mode) {
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 656 case FALLOC_FL_ZERO_RANGE:
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 657 case FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE:
8bd0744b438be1 Sarthak Kukreti 2023-04-19 658 error = truncate_bdev_range(bdev, file->f_mode, start, end) ||
8bd0744b438be1 Sarthak Kukreti 2023-04-19 659 blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT,
6549a874fb65e7 Pavel Begunkov 2021-10-20 660 len >> SECTOR_SHIFT, GFP_KERNEL,
6549a874fb65e7 Pavel Begunkov 2021-10-20 661 BLKDEV_ZERO_NOUNMAP);
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 662 break;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 663 case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE:
8bd0744b438be1 Sarthak Kukreti 2023-04-19 664 error = truncate_bdev_range(bdev, file->f_mode, start, end) ||
8bd0744b438be1 Sarthak Kukreti 2023-04-19 665 blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT,
6549a874fb65e7 Pavel Begunkov 2021-10-20 666 len >> SECTOR_SHIFT, GFP_KERNEL,
6549a874fb65e7 Pavel Begunkov 2021-10-20 667 BLKDEV_ZERO_NOFALLBACK);
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 668 break;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 669 case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE:
8bd0744b438be1 Sarthak Kukreti 2023-04-19 670 error = truncate_bdev_range(bdev, file->f_mode, start, end) ||
8bd0744b438be1 Sarthak Kukreti 2023-04-19 671 blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
44abff2c0b970a Christoph Hellwig 2022-04-15 672 len >> SECTOR_SHIFT, GFP_KERNEL);
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 673 break;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 674 default:
f278eb3d8178f9 Ming Lei 2021-09-23 675 error = -EOPNOTSUPP;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 676 }
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 677
f278eb3d8178f9 Ming Lei 2021-09-23 @678 fail:
f278eb3d8178f9 Ming Lei 2021-09-23 679 filemap_invalidate_unlock(inode->i_mapping);
f278eb3d8178f9 Ming Lei 2021-09-23 680 return error;
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 681 }
cd82cca7ebfe9c Christoph Hellwig 2021-09-07 682
diff --git a/block/fops.c b/block/fops.c index d2e6be4e3d1c..2fd7e8b9ab48 100644 --- a/block/fops.c +++ b/block/fops.c @@ -648,25 +648,27 @@ 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->f_mode, 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 = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, + error = truncate_bdev_range(bdev, file->f_mode, start, end) || + 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 = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT, + error = truncate_bdev_range(bdev, file->f_mode, start, end) || + 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 = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT, + error = truncate_bdev_range(bdev, file->f_mode, start, end) || + blkdev_issue_discard(bdev, start >> SECTOR_SHIFT, len >> SECTOR_SHIFT, GFP_KERNEL); break; default:
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") Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org> --- block/fops.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)