[PATCH 1/3] ext4: Fix an error handling path in ext4_mb_init_cache()

[PATCH 1/3] ext4: Fix an error handling path in ext4_mb_init_cache()

[Christophe JAILLET]

'bhs' is an un-initialized pointer.
If 'groups_per_page' == 1, 'bh' is assigned its address.

Then, in the for loop below, if we early exit, either because
"group >= ngroups" or if ext4_get_group_info() fails, then it is still left
un-initialized.

It can then be used.
NULL tests could fail and lead to unexpected behavior. Also, should the
error handling path be called, brelse() would be passed a potentially
invalid value.

Better safe than sorry, just make sure it is correctly initialized to NULL.

Fixes: c9de560ded61 ("ext4: Add multi block allocator for ext4")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.

The scenario looks possible, but I don't know if it can really happen...
---
 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index b25a27c86696..ff9a124f439b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1285,7 +1285,7 @@ static int ext4_mb_init_cache(struct folio *folio, char *incore, gfp_t gfp)
 	ext4_group_t first_group, group;
 	int first_block;
 	struct super_block *sb;
-	struct buffer_head *bhs;
+	struct buffer_head *bhs = NULL;
 	struct buffer_head **bh = NULL;
 	struct inode *inode;
 	char *data;
-- 
2.47.1

[PATCH 2/3] ext4: Remove some useless assignments in ext4_mb_init_cache()

[Christophe JAILLET]

'bh' assigned an address early in the function and is not used before that.
So the initial NULL assignment can be removed.

Also, if folio_test_uptodate() fails, there is no need to set it to NULL.
'bh' being either kzalloc()'ed or assigned &bhs which is already NULL,
bh[i] is known to be already NULL.

This makes the code more consistent with the other tests above that don't
set this value to NULL.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.
---
 fs/ext4/mballoc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index ff9a124f439b..e536c0e35ca8 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1286,7 +1286,7 @@ static int ext4_mb_init_cache(struct folio *folio, char *incore, gfp_t gfp)
 	int first_block;
 	struct super_block *sb;
 	struct buffer_head *bhs = NULL;
-	struct buffer_head **bh = NULL;
+	struct buffer_head **bh;
 	struct inode *inode;
 	char *data;
 	char *bitmap;
@@ -1330,10 +1330,9 @@ static int ext4_mb_init_cache(struct folio *folio, char *incore, gfp_t gfp)
 		 * which may be currently in use by an allocating task.
 		 */
 		if (folio_test_uptodate(folio) &&
-				!EXT4_MB_GRP_NEED_INIT(grinfo)) {
-			bh[i] = NULL;
+				!EXT4_MB_GRP_NEED_INIT(grinfo))
 			continue;
-		}
+
 		bh[i] = ext4_read_block_bitmap_nowait(sb, group, false);
 		if (IS_ERR(bh[i])) {
 			err = PTR_ERR(bh[i]);
-- 
2.47.1

[PATCH 3/3] ext4: Remove some dead code in the error handling path of ext4_mb_init_cache()

[Christophe JAILLET]

'bh' is known to be a non-NULL value if the error handling path is called,
so there is no need to test for it.

This slighly simplifies the code.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.
---
 fs/ext4/mballoc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e536c0e35ca8..c22f7b8fd941 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1430,12 +1430,11 @@ static int ext4_mb_init_cache(struct folio *folio, char *incore, gfp_t gfp)
 	folio_mark_uptodate(folio);
 
 out:
-	if (bh) {
-		for (i = 0; i < groups_per_page; i++)
-			brelse(bh[i]);
-		if (bh != &bhs)
-			kfree(bh);
-	}
+	for (i = 0; i < groups_per_page; i++)
+		brelse(bh[i]);
+	if (bh != &bhs)
+		kfree(bh);
+
 	return err;
 }
 
-- 
2.47.1

Re: [PATCH 3/3] ext4: Remove some dead code in the error handling path of ext4_mb_init_cache()

[Markus Elfring]

…
> This slighly simplifies the code.

Simplify the source code slightly?

Regards,
Markus

Re: [PATCH 1/3] ext4: Fix an error handling path in ext4_mb_init_cache()

[Dan Carpenter]

On Fri, Jan 03, 2025 at 02:59:16PM +0100, Christophe JAILLET wrote:
> 'bhs' is an un-initialized pointer.
> If 'groups_per_page' == 1, 'bh' is assigned its address.
> 
> Then, in the for loop below, if we early exit, either because
> "group >= ngroups" or if ext4_get_group_info() fails, then it is still left
> un-initialized.
> 
> It can then be used.
> NULL tests could fail and lead to unexpected behavior. Also, should the
> error handling path be called, brelse() would be passed a potentially
> invalid value.
> 
> Better safe than sorry, just make sure it is correctly initialized to NULL.
> 
> Fixes: c9de560ded61 ("ext4: Add multi block allocator for ext4")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only.
> 
> The scenario looks possible, but I don't know if it can really happen...

A pointer to the stack can't ever equal the address of the heap so this
can't happen and it should not have a Fixes tag.

Setting the pointer to NULL probably silences a static checker warning
and these days everyone automatically zeroes stack data so it doesn't
affect the compiled code.  However generally we generally say that we
should fix the checker instead.

I've thought about this in Smatch for a while, and I think what I would
do is say that kmalloc() returns memory that is unique.  Smatch tracks if
variables are equal to each other and unique variables wouldn't be equal
to anything that came earlier.  But I haven't actually tried to implement
this.

regards,
dan carpenter

Re: [PATCH 1/3] ext4: Fix an error handling path in ext4_mb_init_cache()

[Christophe JAILLET]

Le 06/01/2025 à 12:35, Dan Carpenter a écrit :
> On Fri, Jan 03, 2025 at 02:59:16PM +0100, Christophe JAILLET wrote:
>> 'bhs' is an un-initialized pointer.
>> If 'groups_per_page' == 1, 'bh' is assigned its address.
>>
>> Then, in the for loop below, if we early exit, either because
>> "group >= ngroups" or if ext4_get_group_info() fails, then it is still left
>> un-initialized.
>>
>> It can then be used.
>> NULL tests could fail and lead to unexpected behavior. Also, should the
>> error handling path be called, brelse() would be passed a potentially
>> invalid value.
>>
>> Better safe than sorry, just make sure it is correctly initialized to NULL.
>>
>> Fixes: c9de560ded61 ("ext4: Add multi block allocator for ext4")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested only.
>>
>> The scenario looks possible, but I don't know if it can really happen...
> 

Hi Dan,

> A pointer to the stack can't ever equal the address of the heap so this
> can't happen and it should not have a Fixes tag.

Not sure to understand what you mean.

I agree with your statement, but my point is that a pointer in the stack 
(and not *to* the stack) (i.e. 'bhs'), if not initialized, could in 
theory be anything. Let consider its value is 0xdeadbeef.

Then, if groups_per_page == 1, 'bh' points to the stack. Its value is 
"&bhs". And "bh[0]" is 0xdeadbeef.


Should ext4_get_group_info() fail on the first (and only) iteration of 
the for loop, then we 'continue'.
So the loop is done, and bh[0] is never updated, so still points to a 
memory holding 0xdeadbeef.

On the next for loop, on the first (and only) iteration, bh[0] is not 
NULL (it is 0xdeadbeef), so we call:
	ext4_wait_block_bitmap(..., 0xdeadbeef);

If we branch to the error handling path, it would also lead to calling
	brelse(bh[0]), that is to say brelse(0xdeadbeef);


Hoping my analysis is correct, I hope my reasoning is clearer.


That's the theory.
In practice, see below. Certainly harmless thanks to compilers, but 
still a UB for me, so should need a Fixes and a backport (it can't hurt 
anyway) to fix the theory.

> Setting the pointer to NULL probably silences a static checker warning
> and these days everyone automatically zeroes stack data so it doesn't
> affect the compiled code.

Agreed, but unless we have a explicit gcc flag to ask for that behavior 
(I've not checked if it is already the case), it looks like an UB for me.

> However generally we generally say that we
> should fix the checker instead.

In this particular case, the checker is just me, not an static analysis 
tool :).

I looked at this place because one of my coccinelle script spotted:

	/* allocate buffer_heads to read bitmaps */
	if (groups_per_page > 1) {
		i = sizeof(struct buffer_head *) * groups_per_page;
		bh = kzalloc(i, gfp);

as a candidate for kcalloc().

The rest of the story is just by reading the code around it.

> 
> I've thought about this in Smatch for a while, and I think what I would
> do is say that kmalloc() returns memory that is unique.  Smatch tracks if
> variables are equal to each other and unique variables wouldn't be equal
> to anything that came earlier.  But I haven't actually tried to implement
> this.
> 
> regards,
> dan carpenter
> 

Re: [PATCH 1/3] ext4: Fix an error handling path in ext4_mb_init_cache()

[Dan Carpenter]

Ah.  Sorry, I misunderstood the issue.  I can't say for sure whether the
bug is possible but it seems like a reasonable concern.

regards,
dan carpenter