| Message ID | 20190301171504.8583-1-lczerner@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | ext4: Add missing brelse() in add_new_gdb_meta_bg() | expand |
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; > } > >
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
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 >
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; }
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(+)