Re: [PATCH v2 3/3] fs: ext4: widen ext4fs_update() block-group loop counter

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4488 bytes --]

On Tue, Sep 15, 2026 at 02:49:12PM -0700, Sriram Sriram wrote:
> jerome.forissier@arm.com; ankluemk@microsoft.com; v-dmunic@microsoft.com
> Bcc: 
> Subject: Re: [PATCH v2 3/3] fs: ext4: widen ext4fs_update() block-group loop
>  counter
> Reply-To: 
> In-Reply-To: <CAJaLiFxMbEO1hk0o6ja6w1SKN-hVK4tGkvUo90uO+eFgAc631Q@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
> > <sriramsriram@linux.microsoft.com> wrote:
> > >
> > > From: Daniel Munic <v-dmunic@microsoft.com>
> > >
> > > 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 <v-dmunic@microsoft.com>
> > > Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
> > Reviewed-by: Tony Dinh <mibodhi@gmail.com>
> > 
> > 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 <mibodhi@gmail.com>
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]