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@xxxxxxxxxx>
> > ---
> > 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
>
