On Tue, Sep 15, 2026 at 04:12:46PM -0600, Tom Rini wrote: > On Tue, Sep 15, 2026 at 02:49:12PM -0700, Sriram Sriram wrote: > > [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. > > Hi Sriram, your emails keep having some header looking content at the > start of them, what are you using for an email client? Thanks. > > -- > Tom I use mutt. Sorry, I am new to this. I will be careful next time.
Thanks, Sriram
