Re: [PATCH] ext4: Add missing brelse() in add_new_gdb_meta_bg()

[Lukas Czerner <lczerner@redhat.com>]

On Sat, Mar 02, 2019 at 09:07:23PM -0500, Theodore Y. Ts'o wrote:
> On Fri, Mar 01, 2019 at 06:15:04PM +0100, Lukas Czerner wrote:
> > Currently in add_new_gdb_meta_bg() there is a missing brelse of gdb_bh
> > in case ext4_journal_get_write_access() fails. Fix it.
> > 
> > Fixes: 61a9c11e5e7a ("ext4: add missing brelse() add_new_gdb_meta_bg()'s error path")
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  fs/ext4/resize.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> > index 48421de803b7..e945f412cf58 100644
> > --- a/fs/ext4/resize.c
> > +++ b/fs/ext4/resize.c
> > @@ -937,6 +937,8 @@ static int add_new_gdb_meta_bg(struct super_block *sb,
> >  	kvfree(o_group_desc);
> >  	BUFFER_TRACE(gdb_bh, "get_write_access");
> >  	err = ext4_journal_get_write_access(handle, gdb_bh);
> > +	if (err)
> > +		brelse(gdb_bh);
> 
> I believe this isn't the right fix --- or at least, it's not
> sufficient.  We're releasing gdb_bh, but there is still a pointer left
> in n_group_desc[gdb_num] (which is now invalid), and we've already
> replaced o_group_desc with n_group_desc, and incremented s_gdb_count.

Right, I missed that.

> 
> So we should move the call to ext4_journal_get_write_access() earlier
> in the function.
> 
> Ric's comments about checking similar function is also right;
> add_new_gdb() doesn't really get the error handling right, but that's
> an extremely deprecated interface.  We actually had a bug in the old
> resizing ioctl's that was accidentally introduce in 4.4, and no once
> until until December of last year.  (I think it was some crazy user
> with an enterprise distro still using e2fsprogs 1.42, and they tried
> going to a modern kernel, and online resizing didn't work for them.)
> 
> Anyway, while fixing add_new_gdb() might be nice, we can save that for
> another patch and I don't think it's super high priority since it's an
> error handling path for a code path that almost no one uses and was
> broken for two years without no one noticing (although maybe Red Hat
> would prioritize it differently :-).  But could you resend this with
> the call to ext4_journal_get_write_access() moved up earlier in the
> function?

Agreed, I'll resend.

Thanks!
-Lukas

> 
> Thanks!
> 
> 					- Ted
>