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

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

[Sriram Sriram]

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>
---
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

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

[Tony Dinh]

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
>

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

[Sriram Sriram]

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.

Thanks,
Sriram

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

[Tom Rini]

[-- 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 --]

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

[Sriram Sriram]

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:
> > 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
I use mutt. Sorry, I am new to this. I will be careful next time.

Thanks,
Sriram

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

[Tony Dinh]

Hi Sriram,

On Tue, Sep 15, 2026 at 3:17 PM Sriram Sriram
<sriramsriram@linux.microsoft.com> 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:
> > > 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.

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

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

[Sriram Sriram]

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
> <sriramsriram@linux.microsoft.com> 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:
> > > > 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.
> 
> 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