re: ext4: make the bitmap read routines return real error codes

re: ext4: make the bitmap read routines return real error codes

[Dan Carpenter]

Hello Darrick J. Wong,

The patch 7d6232775976: "ext4: make the bitmap read routines return
real error codes" from Oct 17, 2015, leads to the following static
checker warning:

	fs/ext4/mballoc.c:2989 ext4_mb_mark_diskspace_used()
	error: 'bitmap_bh' dereferencing possible ERR_PTR()

fs/ext4/mballoc.c
  2899          bitmap_bh = ext4_read_block_bitmap(sb, ac->ac_b_ex.fe_group);
  2900          if (IS_ERR(bitmap_bh)) {
  2901                  err = PTR_ERR(bitmap_bh);
  2902                  goto out_err;
  2903          }
  2904  

[ snip ]

  2987  
  2988  out_err:
  2989          brelse(bitmap_bh);
  2990          return err;
  2991  }

Also:

fs/ext4/mballoc.c:4894 ext4_free_blocks() error: 'bitmap_bh' dereferencing possible ERR_PTR()
fs/ext4/mballoc.c:5028 ext4_group_add_blocks() error: 'bitmap_bh' dereferencing possible ERR_PTR()

This is One Err style error handling where one error label handles every
possible error so it's error prone (handling every error is more
complicated than doing a specific thing).

The old code relied on the sanity check in brelse() to avoid NULL
dereferences but now we are passing ERR_PTRs so it's not enough.
Probably the fix is to update the sanity check in brelse().  Another
idea would be to not free things until they have been allocated.

regards,
dan carpenter

Re: ext4: make the bitmap read routines return real error codes

[Darrick J. Wong]

On Tue, Nov 03, 2015 at 03:04:45PM +0300, Dan Carpenter wrote:
> Hello Darrick J. Wong,
> 
> The patch 7d6232775976: "ext4: make the bitmap read routines return
> real error codes" from Oct 17, 2015, leads to the following static
> checker warning:
> 
> 	fs/ext4/mballoc.c:2989 ext4_mb_mark_diskspace_used()
> 	error: 'bitmap_bh' dereferencing possible ERR_PTR()
> 
> fs/ext4/mballoc.c
>   2899          bitmap_bh = ext4_read_block_bitmap(sb, ac->ac_b_ex.fe_group);
>   2900          if (IS_ERR(bitmap_bh)) {
>   2901                  err = PTR_ERR(bitmap_bh);
>   2902                  goto out_err;
>   2903          }
>   2904  
> 
> [ snip ]
> 
>   2987  
>   2988  out_err:
>   2989          brelse(bitmap_bh);
>   2990          return err;
>   2991  }
> 
> Also:
> 
> fs/ext4/mballoc.c:4894 ext4_free_blocks() error: 'bitmap_bh' dereferencing possible ERR_PTR()
> fs/ext4/mballoc.c:5028 ext4_group_add_blocks() error: 'bitmap_bh' dereferencing possible ERR_PTR()
> 
> This is One Err style error handling where one error label handles every
> possible error so it's error prone (handling every error is more
> complicated than doing a specific thing).
> 
> The old code relied on the sanity check in brelse() to avoid NULL
> dereferences but now we are passing ERR_PTRs so it's not enough.
> Probably the fix is to update the sanity check in brelse().  Another
> idea would be to not free things until they have been allocated.

Or just slip in a "bitmap_bh = NULL;" just before line 2902.  We've saved
the error code, so the pointer can be zeroed.

Hmm, thank you for the report, I'll get a patch out soon.  Guess I should
go figure out how to smatch-scan my dev tree. :)

(Particularly because I fixed this exact problem in other parts of the patch,
but not here. Sigh.)

--D

> 
> regards,
> dan carpenter