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

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

[Lukas Czerner]

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);
 	return err;
 }
 
-- 
2.20.1

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

[Eric Sandeen]

On 3/1/19 11:15 AM, 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>

I'm going to refrain from actual review because I've gotten fairly
far away from ext4 code, but OTOH I did bring this question to Lukas,
so I'd also like to ask whether this error handling is problematic
in several other callsites... my meager pattern recognition skills
while looking over the code make me go "hmmmm...." a little but
I may well be missing the big picture of how these buffer heads
are managed.

Thanks,
-Eric


> ---
>  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);
>  	return err;
>  }
>  
> 

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

[Theodore Y. Ts'o]

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.

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?

Thanks!

					- Ted
		

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

[Lukas Czerner]

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
>