[PATCH] btrfs: fix the uninitialized btrfs_bio::iter

[PATCH] btrfs: fix the uninitialized btrfs_bio::iter

[Qu Wenruo]

[BUG]
There are reports about compression crash with error injection, mostly
triggering the following ASSERT()s in dec_and_test_compressed_bio():

	ASSERT(btrfs_bio(bio)->iter.bi_size);

The call trace triggered by generic/475 (needs compress mount option)
looks like this:

  assertion failed: btrfs_bio(bio)->iter.bi_size, in fs/btrfs/compression.c:213
  ------------[ cut here ]------------
  kernel BUG at fs/btrfs/ctree.h:3551!
  invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
  CPU: 5 PID: 6548 Comm: fsstress Tainted: G           OE     5.17.0-rc7-custom+ #10
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0010:assertfail.constprop.0+0x18/0x1a [btrfs]
  Call Trace:
   <TASK>
   dec_and_test_compressed_bio.cold+0x16/0x2c [btrfs]
   end_compressed_bio_read+0x37/0x170 [btrfs]
   btrfs_submit_compressed_read+0x803/0x820 [btrfs]
   submit_one_bio+0xc7/0x100 [btrfs]
   btrfs_readpage+0xec/0x130 [btrfs]
   filemap_read_folio+0x53/0xf0
   filemap_get_pages+0x6f3/0xa10
   filemap_read+0x1d6/0x520
   new_sync_read+0x24e/0x360
   vfs_read+0x1a1/0x2a0
   ksys_read+0xc9/0x160
   do_syscall_64+0x3b/0x90
   entry_SYSCALL_64_after_hwframe+0x44/0xae

[CAUSE]
Unlike regular IO path, we will initialize btrfs_bio::iter in
btrfs_map_bio(), for error path, we have to manually initialize
btrfs_bio::iter before calling the endio function.

In above case, due to injected errors, we go to finish_cb: tag directly
without submitting with btrfs_map_bio() call.

This leaves btrfs_bio::iter for the compressed bio uninitialized and
caught by the ASSERT().

[FIX]
Fix it by calling btrfs_bio_save_iter() before we call endio for the
compressed bio.

Please fold this fix into commit "btrfs: make
dec_and_test_compressed_bio() to be split bio compatible".

If needed, I can update the series and resend, but if this is the only
problem, it may be better not to flood the list with 17 patches.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 0bf694038c61..2df034f6194c 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -582,6 +582,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 finish_cb:
 	if (bio) {
 		bio->bi_status = ret;
+		btrfs_bio_save_iter(btrfs_bio(bio));
 		bio_endio(bio);
 	}
 	/* Last byte of @cb is submitted, endio will free @cb */
@@ -924,6 +925,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 finish_cb:
 	if (comp_bio) {
 		comp_bio->bi_status = ret;
+		btrfs_bio_save_iter(btrfs_bio(comp_bio));
 		bio_endio(comp_bio);
 	}
 	/* All bytes of @cb is submitted, endio will free @cb */
-- 
2.35.1

Re: [PATCH] btrfs: fix the uninitialized btrfs_bio::iter

[kernel test robot]

Hi Qu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.17-rc8 next-20220316]
[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]

url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-fix-the-uninitialized-btrfs_bio-iter/20220317-082643
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: arm-randconfig-c002-20220314 (https://download.01.org/0day-ci/archive/20220317/202203171007.1LuEGp3u-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
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/0day-ci/linux/commit/8712b95c5f74e4842f3b19b36417be829b2281b2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Qu-Wenruo/btrfs-fix-the-uninitialized-btrfs_bio-iter/20220317-082643
        git checkout 8712b95c5f74e4842f3b19b36417be829b2281b2
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash fs/btrfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/compression.c: In function 'btrfs_submit_compressed_write':
>> fs/btrfs/compression.c:617:17: error: implicit declaration of function 'btrfs_bio_save_iter'; did you mean 'btrfs_do_write_iter'? [-Werror=implicit-function-declaration]
     617 |                 btrfs_bio_save_iter(btrfs_bio(bio));
         |                 ^~~~~~~~~~~~~~~~~~~
         |                 btrfs_do_write_iter
   cc1: some warnings being treated as errors


vim +617 fs/btrfs/compression.c

   494	
   495	/*
   496	 * worker function to build and submit bios for previously compressed pages.
   497	 * The corresponding pages in the inode should be marked for writeback
   498	 * and the compressed pages should have a reference on them for dropping
   499	 * when the IO is complete.
   500	 *
   501	 * This also checksums the file bytes and gets things ready for
   502	 * the end io hooks.
   503	 */
   504	blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
   505					 unsigned int len, u64 disk_start,
   506					 unsigned int compressed_len,
   507					 struct page **compressed_pages,
   508					 unsigned int nr_pages,
   509					 unsigned int write_flags,
   510					 struct cgroup_subsys_state *blkcg_css,
   511					 bool writeback)
   512	{
   513		struct btrfs_fs_info *fs_info = inode->root->fs_info;
   514		struct bio *bio = NULL;
   515		struct compressed_bio *cb;
   516		u64 cur_disk_bytenr = disk_start;
   517		u64 next_stripe_start;
   518		blk_status_t ret;
   519		int skip_sum = inode->flags & BTRFS_INODE_NODATASUM;
   520		const bool use_append = btrfs_use_zone_append(inode, disk_start);
   521		const unsigned int bio_op = use_append ? REQ_OP_ZONE_APPEND : REQ_OP_WRITE;
   522	
   523		ASSERT(IS_ALIGNED(start, fs_info->sectorsize) &&
   524		       IS_ALIGNED(len, fs_info->sectorsize));
   525		cb = kmalloc(compressed_bio_size(fs_info, compressed_len), GFP_NOFS);
   526		if (!cb)
   527			return BLK_STS_RESOURCE;
   528		refcount_set(&cb->pending_sectors, compressed_len >> fs_info->sectorsize_bits);
   529		cb->status = BLK_STS_OK;
   530		cb->inode = &inode->vfs_inode;
   531		cb->start = start;
   532		cb->len = len;
   533		cb->mirror_num = 0;
   534		cb->compressed_pages = compressed_pages;
   535		cb->compressed_len = compressed_len;
   536		cb->writeback = writeback;
   537		cb->orig_bio = NULL;
   538		cb->nr_pages = nr_pages;
   539	
   540		while (cur_disk_bytenr < disk_start + compressed_len) {
   541			u64 offset = cur_disk_bytenr - disk_start;
   542			unsigned int index = offset >> PAGE_SHIFT;
   543			unsigned int real_size;
   544			unsigned int added;
   545			struct page *page = compressed_pages[index];
   546			bool submit = false;
   547	
   548			/* Allocate new bio if submitted or not yet allocated */
   549			if (!bio) {
   550				bio = alloc_compressed_bio(cb, cur_disk_bytenr,
   551					bio_op | write_flags, end_compressed_bio_write,
   552					&next_stripe_start);
   553				if (IS_ERR(bio)) {
   554					ret = errno_to_blk_status(PTR_ERR(bio));
   555					bio = NULL;
   556					goto finish_cb;
   557				}
   558			}
   559			/*
   560			 * We should never reach next_stripe_start start as we will
   561			 * submit comp_bio when reach the boundary immediately.
   562			 */
   563			ASSERT(cur_disk_bytenr != next_stripe_start);
   564	
   565			/*
   566			 * We have various limits on the real read size:
   567			 * - stripe boundary
   568			 * - page boundary
   569			 * - compressed length boundary
   570			 */
   571			real_size = min_t(u64, U32_MAX, next_stripe_start - cur_disk_bytenr);
   572			real_size = min_t(u64, real_size, PAGE_SIZE - offset_in_page(offset));
   573			real_size = min_t(u64, real_size, compressed_len - offset);
   574			ASSERT(IS_ALIGNED(real_size, fs_info->sectorsize));
   575	
   576			if (use_append)
   577				added = bio_add_zone_append_page(bio, page, real_size,
   578						offset_in_page(offset));
   579			else
   580				added = bio_add_page(bio, page, real_size,
   581						offset_in_page(offset));
   582			/* Reached zoned boundary */
   583			if (added == 0)
   584				submit = true;
   585	
   586			cur_disk_bytenr += added;
   587			/* Reached stripe boundary */
   588			if (cur_disk_bytenr == next_stripe_start)
   589				submit = true;
   590	
   591			/* Finished the range */
   592			if (cur_disk_bytenr == disk_start + compressed_len)
   593				submit = true;
   594	
   595			if (submit) {
   596				if (!skip_sum) {
   597					ret = btrfs_csum_one_bio(inode, bio, start, true);
   598					if (ret)
   599						goto finish_cb;
   600				}
   601	
   602				ret = submit_compressed_bio(fs_info, cb, bio, 0);
   603				if (ret)
   604					goto finish_cb;
   605				bio = NULL;
   606			}
   607			cond_resched();
   608		}
   609		if (blkcg_css)
   610			kthread_associate_blkcg(NULL);
   611	
   612		return 0;
   613	
   614	finish_cb:
   615		if (bio) {
   616			bio->bi_status = ret;
 > 617			btrfs_bio_save_iter(btrfs_bio(bio));
   618			bio_endio(bio);
   619		}
   620		/* Last byte of @cb is submitted, endio will free @cb */
   621		if (cur_disk_bytenr == disk_start + compressed_len)
   622			return ret;
   623	
   624		wait_var_event(cb, refcount_read(&cb->pending_sectors) ==
   625				   (disk_start + compressed_len - cur_disk_bytenr) >>
   626				   fs_info->sectorsize_bits);
   627		/*
   628		 * Even with previous bio ended, we should still have io not yet
   629		 * submitted, thus need to finish manually.
   630		 */
   631		ASSERT(refcount_read(&cb->pending_sectors));
   632		/* Now we are the only one referring @cb, can finish it safely. */
   633		finish_compressed_bio_write(cb);
   634		return ret;
   635	}
   636	

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] btrfs: fix the uninitialized btrfs_bio::iter

[David Sterba]

On Thu, Mar 17, 2022 at 08:23:12AM +0800, Qu Wenruo wrote:
> [BUG]
> There are reports about compression crash with error injection, mostly
> triggering the following ASSERT()s in dec_and_test_compressed_bio():
> 
> 	ASSERT(btrfs_bio(bio)->iter.bi_size);
> 
> The call trace triggered by generic/475 (needs compress mount option)
> looks like this:
> 
>   assertion failed: btrfs_bio(bio)->iter.bi_size, in fs/btrfs/compression.c:213
>   ------------[ cut here ]------------
>   kernel BUG at fs/btrfs/ctree.h:3551!
>   invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
>   CPU: 5 PID: 6548 Comm: fsstress Tainted: G           OE     5.17.0-rc7-custom+ #10
>   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
>   RIP: 0010:assertfail.constprop.0+0x18/0x1a [btrfs]
>   Call Trace:
>    <TASK>
>    dec_and_test_compressed_bio.cold+0x16/0x2c [btrfs]
>    end_compressed_bio_read+0x37/0x170 [btrfs]
>    btrfs_submit_compressed_read+0x803/0x820 [btrfs]
>    submit_one_bio+0xc7/0x100 [btrfs]
>    btrfs_readpage+0xec/0x130 [btrfs]
>    filemap_read_folio+0x53/0xf0
>    filemap_get_pages+0x6f3/0xa10
>    filemap_read+0x1d6/0x520
>    new_sync_read+0x24e/0x360
>    vfs_read+0x1a1/0x2a0
>    ksys_read+0xc9/0x160
>    do_syscall_64+0x3b/0x90
>    entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> [CAUSE]
> Unlike regular IO path, we will initialize btrfs_bio::iter in
> btrfs_map_bio(), for error path, we have to manually initialize
> btrfs_bio::iter before calling the endio function.
> 
> In above case, due to injected errors, we go to finish_cb: tag directly
> without submitting with btrfs_map_bio() call.
> 
> This leaves btrfs_bio::iter for the compressed bio uninitialized and
> caught by the ASSERT().
> 
> [FIX]
> Fix it by calling btrfs_bio_save_iter() before we call endio for the
> compressed bio.
> 
> Please fold this fix into commit "btrfs: make
> dec_and_test_compressed_bio() to be split bio compatible".
> 
> If needed, I can update the series and resend, but if this is the only
> problem, it may be better not to flood the list with 17 patches.

No need to resend, fixup folded to the patch and the series will be in
for-next again, thanks.