[email protected]; [email protected]; [email protected] Bcc: Subject: Re: [PATCH v2 3/3] fs: ext4: widen ext4fs_update() block-group loop counter Reply-To: In-Reply-To: <cajalifxmbeo1hk0o6ja6w1skn-hvk4tgkvuo90uo+efgac6...@mail.gmail.com>
On Tue, Sep 15, 2026 at 02:23:12PM -0700, Tony Dinh wrote: > Hi Sriram and Daniel, > > On Tue, Sep 15, 2026 at 1:52 PM Sriram Sriram > <[email protected]> wrote: > > > > From: Daniel Munic <[email protected]> > > > > ext4fs_update() iterates over all block groups with a signed 16-bit > > loop counter: > > > > short i; > > ... > > for (i = 0; i < fs->no_blkgrp; i++) > > > > fs->no_blkgrp is a uint32_t. On a filesystem with more than 32767 > > block groups the counter cannot represent every index: incrementing > > past SHRT_MAX is signed overflow (undefined behaviour) and the > > comparison against the unsigned no_blkgrp never terminates correctly, > > so the bitmap/group-descriptor write-back loops misbehave. The mixed > > signed/unsigned comparison is also flagged by static analysis. > > > > Use u32 for the loop counter, matching the width of no_blkgrp. > > > > Signed-off-by: Daniel Munic <[email protected]> > > Signed-off-by: Sriram Sriram <[email protected]> > Reviewed-by: Tony Dinh <[email protected]> > > BTW, since you are testing this ext4_write, it might be worthwhile to > see this commit: > https://github.com/u-boot/u-boot/commit/53cc4332b3b37218a7cdab8bdb953da57eec2668 > > Thanks, > Tony > > > --- > > Changes in v2: > > - No change, rebased onto current master. > > > > fs/ext4/ext4_write.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c > > index 1abedcede72..2fba2197c19 100644 > > --- a/fs/ext4/ext4_write.c > > +++ b/fs/ext4/ext4_write.c > > @@ -67,7 +67,7 @@ static inline void ext4fs_bg_free_blocks_inc > > > > static void ext4fs_update(void) > > { > > - short i; > > + u32 i; > > ext4fs_update_journal(); > > struct ext_filesystem *fs = get_fs(); > > struct ext2_block_group *bgd = NULL; > > -- > > 2.49.0 > > > Reviewed-by: Tony Dinh <[email protected]> Hi Tony, Thanks for the review. > BTW, since you are testing this ext4_write, it might be worthwhile to > see this commit: > ext4fs: Fix: Read outside partition error (take 2) Thanks for the pointer. 53cc4332b3b is already in the tree this series is based on, so the read path is covered here. What caught my eye is the note in that commit message: Looks like we also have this overflown problem in ext4_write.c that needs to be addressed. As far as I can tell that is still unaddressed. ext4fs_write_file() has the same wrap you fixed in ext4fs_read_file(): long int blknr; ... blknr = blknr << log2_fs_blocksize; and on top of that delayed_start, delayed_next and previous_block_number are all plain int: int previous_block_number = -1; int delayed_start = 0; int delayed_next = 0; which makes the casts at the put_ext4() call sites put_ext4((uint64_t)((uint64_t)delayed_start << log2blksz), delayed_buf, (uint32_t)delayed_extent); ineffective, since the value has already been truncated on its way into delayed_start. The fix would mirror 53cc4332b3b: lbaint_t blknr plus a separate long for the read_allocated_block() return and error check, and widening delayed_start/delayed_next to lbaint_t. I am happy to write that up, but I should be up front that I cannot test it. The largest medium I can reach here is well below the 2^31 sector point, and since this is the write path an untested patch risks writing to the wrong LBA. Would you rather take it yourself, given you already have the DS116 and the 4TB disk set up from the read-side reproducer? If you would prefer that I send it, I can post it with you on Cc and leave it pending your Tested-by. Thanks, Sriram
