| Message ID | 20231218141814.1477338-2-libaokun1@huawei.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ext4: fix divide error in mb_update_avg_fragment_size() | expand |
On Mon 18-12-23 22:18:11, Baokun Li wrote: > In ext4_move_extents(), moved_len is only updated when all moves are > successfully executed, and only discards orig_inode and donor_inode > preallocations when moved_len is not zero. When the loop fails to exit > after successfully moving some extents, moved_len is not updated and > remains at 0, so it does not discard the preallocations. > > If the moved extents overlap with the preallocated extents, the > overlapped extents are freed twice in ext4_mb_release_inode_pa() and > ext4_process_freed_data() (as described in commit 94d7c16cbbbd), and > bb_free is incremented twice. Hence when trim is executed, a zero-division > bug is triggered in mb_update_avg_fragment_size() because bb_free is not > zero and bb_fragments is zero. > > Therefore, update move_len after each extent move to avoid the issue. > > Reported-by: Wei Chen <harperchen1110@gmail.com> > Reported-by: xingwei lee <xrivendell7@gmail.com> > Closes: https://lore.kernel.org/r/CAO4mrferzqBUnCag8R3m2zf897ts9UEuhjFQGPtODT92rYyR2Q@mail.gmail.com > Fixes: fcf6b1b729bc ("ext4: refactor ext4_move_extents code base") > CC: stable@vger.kernel.org # 3.18 > Signed-off-by: Baokun Li <libaokun1@huawei.com> > --- > fs/ext4/move_extent.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c > index 3aa57376d9c2..4b9b503c6346 100644 > --- a/fs/ext4/move_extent.c > +++ b/fs/ext4/move_extent.c > @@ -672,7 +672,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, > */ > ext4_double_up_write_data_sem(orig_inode, donor_inode); > /* Swap original branches with new branches */ > - move_extent_per_page(o_filp, donor_inode, > + *moved_len += move_extent_per_page(o_filp, donor_inode, > orig_page_index, donor_page_index, > offset_in_page, cur_len, > unwritten, &ret); Although this is currently fine, I think ext4_move_extents() should be careful and zero out *moved_len before it starts using it. > @@ -682,7 +682,6 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, > o_start += cur_len; > d_start += cur_len; > } > - *moved_len = o_start - orig_blk; > if (*moved_len > len) > *moved_len = len; So I'm not sure the *moved_len > len condition can ever trigger but if it does, we'd need to check it whenever we are returning moved_len. So either I'd delete the condition or move it to the out label. Honza
On 2023/12/18 23:32, Jan Kara wrote: > On Mon 18-12-23 22:18:11, Baokun Li wrote: >> In ext4_move_extents(), moved_len is only updated when all moves are >> successfully executed, and only discards orig_inode and donor_inode >> preallocations when moved_len is not zero. When the loop fails to exit >> after successfully moving some extents, moved_len is not updated and >> remains at 0, so it does not discard the preallocations. >> >> If the moved extents overlap with the preallocated extents, the >> overlapped extents are freed twice in ext4_mb_release_inode_pa() and >> ext4_process_freed_data() (as described in commit 94d7c16cbbbd), and >> bb_free is incremented twice. Hence when trim is executed, a zero-division >> bug is triggered in mb_update_avg_fragment_size() because bb_free is not >> zero and bb_fragments is zero. >> >> Therefore, update move_len after each extent move to avoid the issue. >> >> Reported-by: Wei Chen <harperchen1110@gmail.com> >> Reported-by: xingwei lee <xrivendell7@gmail.com> >> Closes: https://lore.kernel.org/r/CAO4mrferzqBUnCag8R3m2zf897ts9UEuhjFQGPtODT92rYyR2Q@mail.gmail.com >> Fixes: fcf6b1b729bc ("ext4: refactor ext4_move_extents code base") >> CC: stable@vger.kernel.org # 3.18 >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> --- >> fs/ext4/move_extent.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c >> index 3aa57376d9c2..4b9b503c6346 100644 >> --- a/fs/ext4/move_extent.c >> +++ b/fs/ext4/move_extent.c >> @@ -672,7 +672,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, >> */ >> ext4_double_up_write_data_sem(orig_inode, donor_inode); >> /* Swap original branches with new branches */ >> - move_extent_per_page(o_filp, donor_inode, >> + *moved_len += move_extent_per_page(o_filp, donor_inode, >> orig_page_index, donor_page_index, >> offset_in_page, cur_len, >> unwritten, &ret); > Although this is currently fine, I think ext4_move_extents() should be > careful and zero out *moved_len before it starts using it. Totally agree, now __ext4_ioctl zeroes it out, but to avoid code logic changes or new callers afterward, I'll zero it out before using it in ext4_move_extents() in the next version. > >> @@ -682,7 +682,6 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, >> o_start += cur_len; >> d_start += cur_len; >> } >> - *moved_len = o_start - orig_blk; >> if (*moved_len > len) >> *moved_len = len; > So I'm not sure the *moved_len > len condition can ever trigger but if it > does, we'd need to check it whenever we are returning moved_len. So either > I'd delete the condition or move it to the out label. > > Honza As the code stands now, the *moved_len > len condition never occurs, and is supposed to be code left over from the refactoring in [Fixes], which I'll remove in the next version. Thank you for your review!
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c index 3aa57376d9c2..4b9b503c6346 100644 --- a/fs/ext4/move_extent.c +++ b/fs/ext4/move_extent.c @@ -672,7 +672,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, */ ext4_double_up_write_data_sem(orig_inode, donor_inode); /* Swap original branches with new branches */ - move_extent_per_page(o_filp, donor_inode, + *moved_len += move_extent_per_page(o_filp, donor_inode, orig_page_index, donor_page_index, offset_in_page, cur_len, unwritten, &ret); @@ -682,7 +682,6 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk, o_start += cur_len; d_start += cur_len; } - *moved_len = o_start - orig_blk; if (*moved_len > len) *moved_len = len;
In ext4_move_extents(), moved_len is only updated when all moves are successfully executed, and only discards orig_inode and donor_inode preallocations when moved_len is not zero. When the loop fails to exit after successfully moving some extents, moved_len is not updated and remains at 0, so it does not discard the preallocations. If the moved extents overlap with the preallocated extents, the overlapped extents are freed twice in ext4_mb_release_inode_pa() and ext4_process_freed_data() (as described in commit 94d7c16cbbbd), and bb_free is incremented twice. Hence when trim is executed, a zero-division bug is triggered in mb_update_avg_fragment_size() because bb_free is not zero and bb_fragments is zero. Therefore, update move_len after each extent move to avoid the issue. Reported-by: Wei Chen <harperchen1110@gmail.com> Reported-by: xingwei lee <xrivendell7@gmail.com> Closes: https://lore.kernel.org/r/CAO4mrferzqBUnCag8R3m2zf897ts9UEuhjFQGPtODT92rYyR2Q@mail.gmail.com Fixes: fcf6b1b729bc ("ext4: refactor ext4_move_extents code base") CC: stable@vger.kernel.org # 3.18 Signed-off-by: Baokun Li <libaokun1@huawei.com> --- fs/ext4/move_extent.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)