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
signature.asc
Description: PGP signature
