lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20230216130305.nrbtd42tppxhbynn@quack3>
Date:   Thu, 16 Feb 2023 14:03:05 +0100
From:   Jan Kara <jack@...e.cz>
To:     zhanchengbin <zhanchengbin1@...wei.com>
Cc:     Jan Kara <jack@...e.cz>, tytso@....edu, jack@...e.com,
        linux-ext4@...r.kernel.org, yi.zhang@...wei.com,
        linfeilong@...wei.com, liuzhiqiang26@...wei.com,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v5 2/2] ext4: clear the verified flag of the modified
 leaf or idx if error

On Thu 16-02-23 15:25:23, zhanchengbin wrote:
> The last patch did not take into account path[0].p_bh == NULL, so I
> reworked the code.
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 0f95e857089e..05585afae0db 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1750,13 +1750,19 @@ static int ext4_ext_correct_indexes(handle_t
> *handle, struct inode *inode,
>                         break;
>                 err = ext4_ext_get_access(handle, inode, path + k);
>                 if (err)
> -                       break;
> +                       goto clean;
>                 path[k].p_idx->ei_block = border;
>                 err = ext4_ext_dirty(handle, inode, path + k);
>                 if (err)
> -                       break;
> +                       goto clean;
>         }
> +       return 0;
> 
> +clean:
> +       while (k++ < depth) {
> +               /* k here will not be 0, so don't consider the case where
> path[0].p_bh is NULL */

Please avoid the line over 80 characters.

> +               clear_buffer_verified(path[k].p_bh);
> +       }
>         return err;
>  }
> 
> @@ -2304,6 +2310,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct
> inode *inode,
>  {
>         int err;
>         ext4_fsblk_t leaf;
> +       int b_depth = depth;
> 
>         /* free index block */
>         depth--;
> @@ -2339,11 +2346,18 @@ static int ext4_ext_rm_idx(handle_t *handle, struct
> inode *inode,
>                 path--;
>                 err = ext4_ext_get_access(handle, inode, path);
>                 if (err)
> -                       break;
> +                       goto clean;
>                 path->p_idx->ei_block = (path+1)->p_idx->ei_block;
>                 err = ext4_ext_dirty(handle, inode, path);
>                 if (err)
> -                       break;
> +                       goto clean;
> +       }
> +       return 0;
> +
> +clean:
> +       while (depth++ < b_depth - 1) {
> +               /* depth here will not be 0, so don't consider the case
> where path[0].p_bh is NULL */

Again please avoid the overly long line.

> +               clear_buffer_verified(path[depth].p_bh);
>         }

I think this is still problematic because 'path' is being updated in the
above loop as well so this will still access beyond the end of the array.
So I think you first need to modify ext4_ext_rm_idx() to leave 'path' alone
and just index it like ext4_ext_correct_indexes() does it (separate patch
please) and then add this error recovery path.

> On 2023/2/14 20:52, Jan Kara wrote:
> > 
> > This would be more understandable as:
> > 
> > 	if (k >= 0)
> > 		while (k++ < depth)
> > 			...
> > 
> > Also the loop is IMO wrong because it will run with k == depth as well (due
> > to post-increment) and that is not initialized. Furthermore it will run
> > also if we exit the previous loop due to:
> > 
> >                  /* change all left-side indexes */
> >                  if (path[k+1].p_idx != EXT_FIRST_INDEX(path[k+1].p_hdr))
> >                          break;
> > 
> > which is unwanted as well. Which suggests that you didn't test your changes
> > much (if at all...). So please make sure your changes are tested next time.
> > Thank you!
> > 
> I only ran xfstest locally. Do you have any better suggestions?

Yes that's good. But that will not run your new error handling code at all,
will it? It would be good if you also ran the reproducer that presumably
triggered these fixes to exercise the new code...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ