On Tue, Sep 15, 2026 at 03:41:32PM -0700, Tony Dinh wrote: > Hi Sriram, > > On Tue, Sep 15, 2026 at 3:17 PM Sriram Sriram > <[email protected]> wrote: > > > > 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. > > Unfortunately, I don't have any free 4TB HDD left to test with at the > moment either! That 4TB HDD is no longer available. > > All the best, > Tony > > > > > > > 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
> Unfortunately, I don't have any free 4TB HDD left to test with at the > moment either! That 4TB HDD is no longer available. > > All the best, > Tony Okay, I will ignore that part. Thanks, Sriram
