2012-10-25 11:35:11

by Dan Carpenter

Subject: re: ext4: Replace BUG_ON() with ext4_error() in move_extents.c

Hello Akira Fujita,

This is a semi-automatic email about new static checker warnings.

The patch 2147b1a6a48e: "ext4: Replace BUG_ON() with ext4_error() in
move_extents.c" from Sep 16, 2009, leads to the following Smatch
complaint:

fs/ext4/move_extent.c:693 mext_replace_branches()
warn: variable dereferenced before check 'dext' (see line 683)

fs/ext4/move_extent.c
682 dext = donor_path[depth].p_ext;
683 tmp_dext = *dext;
^^^^^
Old dereference.

684
685 *err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,
686 donor_off, count);
687 if (*err)
688 goto out;
689
690 /* Loop for the donor extents */
691 while (1) {
692 /* The extent for donor must be found. */
693 if (!dext) {
^^^^^
This check was inside BUG_ON() macro before and so Smatch ignored it on
the basis that macros do a lot of unneeded checks. But now it's outside
the macro it triggers a warning.

694 EXT4_ERROR_INODE(donor_inode,
695 "The extent for donor must be found");

regards,
dan carpenter

2012-10-26 08:13:55

by Akira Fujita

Subject: Re: ext4: Replace BUG_ON() with ext4_error() in move_extents.c

Hello Dan,

Thanks for reporting,
this patch removes smatch warning.

Signed-off-by: Akria Fujita <[email protected]>
---
fs/ext4/move_extent.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 292daee..17d28a1 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -680,6 +680,10 @@ mext_replace_branches(handle_t *handle, struct inode *orig_inode,

depth = ext_depth(donor_inode);
dext = donor_path[depth].p_ext;
+ if (unlikely(!dext)) {
+ *err = -EIO;
+ goto out;
+ }
tmp_dext = *dext;

*err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,

Regards,
Akira Fujita

(2012/10/25 20:35), Dan Carpenter wrote:
> Hello Akira Fujita,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 2147b1a6a48e: "ext4: Replace BUG_ON() with ext4_error() in
> move_extents.c" from Sep 16, 2009, leads to the following Smatch
> complaint:
>
> fs/ext4/move_extent.c:693 mext_replace_branches()
> warn: variable dereferenced before check 'dext' (see line 683)
>
> fs/ext4/move_extent.c
> 682 dext = donor_path[depth].p_ext;
> 683 tmp_dext = *dext;
> ^^^^^
> Old dereference.
>
> 684
> 685 *err = mext_calc_swap_extents(&tmp_dext, &tmp_oext, orig_off,
> 686 donor_off, count);
> 687 if (*err)
> 688 goto out;
> 689
> 690 /* Loop for the donor extents */
> 691 while (1) {
> 692 /* The extent for donor must be found. */
> 693 if (!dext) {
> ^^^^^
> This check was inside BUG_ON() macro before and so Smatch ignored it on
> the basis that macros do a lot of unneeded checks. But now it's outside
> the macro it triggers a warning.
>
> 694 EXT4_ERROR_INODE(donor_inode,
> 695 "The extent for donor must be found");
>
> regards,
> dan carpenter
>
>