Re: [PATCH] Btrfs: fix unexpected balance crash due to BUG_ON

[Liu Bo <bo.li.liu@oracle.com>]

On Wed, May 04, 2016 at 01:14:27AM +0200, Holger Hoffstätte wrote:
> On Tue, May 3, 2016 at 1:01 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > Mounting a btrfs can resume previous balance operations asynchronously.
> > An user got a crash when one drive has some corrupt sectors.
> >
> > Since balance can cancel itself in case of any error, we can gracefully
> > return errors to upper layers and let balance do the cancel job.
> >
> > Reported-by: sash <master.b.at.raven@chefmail.de>
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/volumes.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index bd0f45f..5aed2e2 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3418,13 +3418,25 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >                 ret = btrfs_shrink_device(device, old_size - size_to_free);
> >                 if (ret == -ENOSPC)
> >                         break;
> > -               BUG_ON(ret);
> > +               if (ret) {
> > +                       /* btrfs_shrink_device never returns ret > 0 */
> > +                       WARN_ON_ONCE(ret > 0);
> > +                       goto error;
> > +               }
> >
> >                 trans = btrfs_start_transaction(dev_root, 0);
> > -               BUG_ON(IS_ERR(trans));
> > +               if (IS_ERR(trans)) {
> > +                       ret = PTR_ERR(trans);
> > +                       goto error;
> > +               }
> >
> >                 ret = btrfs_grow_device(trans, device, old_size);
> > -               BUG_ON(ret);
> > +               if (ret) {
> > +                       btrfs_end_transaction(trans, dev_root);
> > +                       /* btrfs_grow_device never returns ret > 0 */
> > +                       WARN_ON_ONCE(ret > 0);
> > +                       goto error;
> > +               }
> >
> >                 btrfs_end_transaction(trans, dev_root);
> >         }
> 
> Just a heads up that this seems to introduce a valid warning, since it now
> can goto error before the first initializing use of path:
> 
> fs/btrfs/volumes.c: In function 'btrfs_balance':
> fs/btrfs/volumes.c:3601:2: warning: 'path' may be used uninitialized
> in this function [-Wmaybe-uninitialized]
>   btrfs_free_path(path);
>   ^
> fs/btrfs/volumes.c:3385:21: note: 'path' was declared here
>   struct btrfs_path *path;
>                      ^
> (it's really in __btrfs_balance which got inlined, so gcc thinks it's
> at the call site).
> 
> Simply setting path = NULL at the beginning of __btrfs_balance fixes it, since
> btrfs_free_path allows NULL values.

That's right, it's weird that I didn't get this warning while testing it.

Thanks for catching it, Holger.

Thanks,

-liubo