On 2023/7/21 3:30, Ritesh Harjani (IBM) wrote:
I would like to carefully review all such paths. I will soon review and
get back.
Okay, thank you very much for your careful review.
The 2nd and 3rd cases of adjusting the best extent are impossible to
overflow,
so only the first case is converted here.
I noticed them too during review. I think it would be safe to make the
changes in other two places as well such that in future we never
trip over such overlooked overflow bugs.
+ BUG_ON(new_bex_end >
+ fex_end(sbi, &ac->ac_g_ex, &ac->ac_orig_goal_len));
I am not sure whether using fex_end or pa_end is any helpful.
I think we can just typecast if needed and keep it simple rather
than adding helpers functions for addition operation.
(because of the fact that fex_end() can take a third parameter which
sometimes you pass as NULL. Hence it doesn't look clean, IMO)
I added the helper functions here for two reasons:
1. restricting the type of the return value.
2. This avoids the ugly line breaks in most cases.
The fex_end() indeed doesn't look as clean as the pa_end(), because we
might use
the start of the free extent plus some other length to get a new end,
like right in
ext4_mb_new_inode_pa(), which makes me have to add another extra length
argument, but I think it's worth it, and even with the addition of a
parameter
that will probably be unused, it still looks a lot shorter than the
original code.
IMO, we don't need pa_end() and fex_end() at all. In several places in
ext4 we always have taken care by directly typecasting to avoid
overflows. Also it reads much simpler rather to typecast in place than
having a helper function which is also not very elegant due to a third
parameter. Hence I think we should drop those helpers.
I still think helper is useful, but my previous thinking is problematic.
I shouldn't
make fex_end() adapt to ext4_mb_new_inode_pa(), but should make
ext4_mb_new_inode_pa() adapt to fex_end(). After dropping the third argument
of fex_end(), modify the ext4_mb_new_inode_pa() function as follows:
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a2475b8c9fb5..7492ba9062f4 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5072,8 +5072,11 @@ ext4_mb_new_inode_pa(struct
ext4_allocation_context *ac)
pa = ac->ac_pa;
if (ac->ac_b_ex.fe_len < ac->ac_orig_goal_len) {
- int new_bex_start;
- int new_bex_end;
+ struct ext4_free_extent ex = {
+ .fe_logical = ac->ac_g_ex.fe_logical;
+ .fe_len = ac->ac_orig_goal_len;
+ }
+ loff_t orig_goal_end = fex_end(sbi, &ex);
/* we can't allocate as much as normalizer wants.
* so, found space must get proper lstart
@@ -5092,29 +5095,23 @@ ext4_mb_new_inode_pa(struct
ext4_allocation_context *ac)
* still cover original start
* 3. Else, keep the best ex at start of original request.
*/
- new_bex_end = ac->ac_g_ex.fe_logical +
- EXT4_C2B(sbi, ac->ac_orig_goal_len);
- new_bex_start = new_bex_end - EXT4_C2B(sbi,
ac->ac_b_ex.fe_len);
- if (ac->ac_o_ex.fe_logical >= new_bex_start)
- goto adjust_bex;
+ ex.fe_len = ac->ac_b_ex.fe_len;
- new_bex_start = ac->ac_g_ex.fe_logical;
- new_bex_end =
- new_bex_start + EXT4_C2B(sbi, ac->ac_b_ex.fe_len);
- if (ac->ac_o_ex.fe_logical < new_bex_end)
+ ex.fe_logical = orig_goal_end - EXT4_C2B(sbi, ex.fe_len);
+ if (ac->ac_o_ex.fe_logical >= ex.fe_logical)
goto adjust_bex;
- new_bex_start = ac->ac_o_ex.fe_logical;
- new_bex_end =
- new_bex_start + EXT4_C2B(sbi, ac->ac_b_ex.fe_len);
+ ex.fe_logical = ac->ac_g_ex.fe_logical;
+ if (ac->ac_o_ex.fe_logical < fex_end(sbi, &ex))
+ goto adjust_bex;
+ ex.fe_logical = ac->ac_o_ex.fe_logical;
adjust_bex:
- ac->ac_b_ex.fe_logical = new_bex_start;
+ ac->ac_b_ex.fe_logical = ex.fe_logical;
BUG_ON(ac->ac_o_ex.fe_logical < ac->ac_b_ex.fe_logical);
BUG_ON(ac->ac_o_ex.fe_len > ac->ac_b_ex.fe_len);
- BUG_ON(new_bex_end > (ac->ac_g_ex.fe_logical +
- EXT4_C2B(sbi, ac->ac_orig_goal_len)));
+ BUG_ON(fex_end(sbi, &ex) > orig_goal_end);
}
pa->pa_lstart = ac->ac_b_ex.fe_logical;
What do you think of this modification?
Thanks once again for catching the overflows and coming up with a
easy reproducer. I am surprised that this bug was never caught with LTP,
fstests, smatch static checker.
How did you find it? :)
-ritesh
This problem is found in the internal test.
Thank you for your review!
--
With Best Regards,
Baokun Li
.
