Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Dec 2022 17:21:16 -0500
From:   Eric Whitney <enwlinux@gmail.com>
To:     Ye Bin <yebin@huaweicloud.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        jack@suse.cz, Ye Bin <yebin10@huawei.com>,
        syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
Subject: Re: [PATCH v2 1/3] ext4: fix incorrect calculate 'reserved' in
 '__es_remove_extent' when enable bigalloc feature
Message-ID: <Y4ko3OL57iyiRC0W@debian-BULLSEYE-live-builder-AMD64>
References: <20221121121434.1061725-1-yebin@huaweicloud.com>
 <20221121121434.1061725-2-yebin@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221121121434.1061725-2-yebin@huaweicloud.com>
Precedence: bulk

* Ye Bin <yebin@huaweicloud.com>:
> From: Ye Bin <yebin10@huawei.com>
> 
> Syzbot report issue as follows:
> EXT4-fs error (device loop0): ext4_validate_block_bitmap:398: comm rep: bg 0: block 5: invalid block bitmap
> EXT4-fs (loop0): Delayed block allocation failed for inode 18 at logical offset 0 with max blocks 32 with error 28
> EXT4-fs (loop0): This should not happen!! Data will be lost
> 
> EXT4-fs (loop0): Total free blocks count 0
> EXT4-fs (loop0): Free/Dirty block details
> EXT4-fs (loop0): free_blocks=0
> EXT4-fs (loop0): dirty_blocks=32
> EXT4-fs (loop0): Block reservation details
> EXT4-fs (loop0): i_reserved_data_blocks=2
> EXT4-fs (loop0): Inode 18 (00000000845cd634): i_reserved_data_blocks (1) not cleared!
> 
> Above issue happens as follows:
> Assume:
> sbi->s_cluster_ratio = 16
> Step1: Insert delay block [0, 31] -> ei->i_reserved_data_blocks=2
> Step2:
> ext4_writepages
>   mpage_map_and_submit_extent -> return failed
>   mpage_release_unused_pages -> to release [0, 30]
>     ext4_es_remove_extent -> remove lblk=0 end=30
>       __es_remove_extent -> len1=0 len2=31-30=1
>  __es_remove_extent:
>  ...
>  if (len2 > 0) {
>   ...
> 	  if (len1 > 0) {
> 		  ...
> 	  } else {
> 		es->es_lblk = end + 1;
> 		es->es_len = len2;
> 		...
> 	  }
>   	if (count_reserved)
> 		count_rsvd(inode, lblk, orig_es.es_len - len1 - len2, &orig_es, &rc);
> 	goto out; -> will return but didn't calculate 'reserved'
>  ...
> Step3: ext4_destroy_inode -> trigger "i_reserved_data_blocks (1) not cleared!"
> 
> To solve above issue if 'len2>0' call 'get_rsvd()' before goto out.
> 
> Reported-by: syzbot+05a0f0ccab4a25626e38@syzkaller.appspotmail.com
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  fs/ext4/extents_status.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index cd0a861853e3..4684eaea9471 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1371,7 +1371,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  		if (count_reserved)
>  			count_rsvd(inode, lblk, orig_es.es_len - len1 - len2,
>  				   &orig_es, &rc);
> -		goto out;
> +		goto count;
>  	}
>  
>  	if (len1 > 0) {
> @@ -1413,6 +1413,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  		}
>  	}
>  
> +count:
>  	if (count_reserved)
>  		*reserved = get_rsvd(inode, end, es, &rc);
>  out:
> -- 
> 2.31.1
> 

I'm unable to find the sysbot report for this patch, so I can't verify that
this fix works.  The more serious problem would be whatever is causing
the invalid block bitmap and delayed allocation failure messages before the
i_reserved_data_blocks message.  Perhaps that's simply what syzkaller set
up, but it's not clear from this posting.  Have you looked for the cause
of those first two messages?

However, by inspection this patch should fix an obvious bug causing that last
message, introduced by 8fcc3a580651 ("ext4: rework reserved cluster accounting
when invalidating pages").  A Fixes tag should be added to the patch.  Also,
the readability of the code should be improved by changing the label "count" to
the more descriptive "out_get_reserved".

With those two changes, feel free to add:

Reviewed-by: Eric Whitney <enwlinux@gmail.com>

Eric