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

[Dan Carpenter <dan.carpenter@linaro.org>]

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