Re: [PATCH] gfs2: Fix use-after-free in gfs2_remove_from_journal()

[Andreas Gruenbacher]

On Tue, Aug 25, 2026 at 3:36 AM Hongling Zeng <zhongling0719@xxxxxxx> wrote:
> 在 2026年08月24日 23:35, Andreas Gruenbacher 写道:
> > Hi Hongling Zeng,
> >
> > On Thu, Jun 18, 2026 at 3:58 AM Hongling Zeng <zhongling0719@xxxxxxx> wrote:
> >> 在 2026年06月17日 18:51, Andrew Price 写道:
> >>> On 17/06/2026 10:01, Hongling Zeng wrote:
> >>>> The function calls brelse(bh) but then continues to access
> >>>> the buffer head through bh->b_private, clear_buffer_dirty(),
> >>>> and clear_buffer_uptodate().
> >>>>
> >>>> After brelse() decreases the reference count, the buffer head
> >>>> may be freed, making the subsequent accesses use-after-free.
> >>> When buffers are pinned their refcount is incremented and the brelse() here is only called for pinned buffers so I'm not convinced that there's a bug.
> >>>
> >>> Callers of gfs2_remove_from_journal() also use the bh afterwards so if there was a use-after-free this patch wouldn't fix it.
> >>>
> >>> Did you see a use-after-free in testing?
> >>>
> >>> Andy
> >>>
> >>    Thanks for your detailed explanation!  this is not a real bug in
> >> practice, the reference counting protects against real UAF.
> >>    I'm seeing smatch warnings :
> >>
> >>         fs/gfs2/log.c:1044 error: dereferencing freed memory 'bh'
> >>         fs/gfs2/log.c:1051 warn: passing freed memory 'bh'
> > a "make C=2 CHECK="smatch" fs/gfs2/" doesn't result in those messages.
> > How can I reproduce?
> >
> > Thanks,
> > Andreas
> >
>    Hi Andreas and Andy,
>
>    Thank you for the review. The smatch warnings were reported by the Intel
>    LKP test robot:
>
>      https://lore.kernel.org/all/202607110908.l7Owhul4-lkp@xxxxxxxxx/
>
>    I will update the patch with a better commit message based on your feedback,
>    clarifying that this is a defensive improvement to address the pattern
>    flagged by static analysis, rather than fixing a real-world use-after-free
>    bug.
>
>    I'll send a v2 version.

I still cannot reproduce that result with smatch 0.6.4 (up from
v0.5.0-9185-gbcc58b9c) on Fedora 44, but I can live with this patch.
Thanks!

Andreas

> >>     And there are potential concerns:
> >>     1. Future maintainers might not understand the ref counting semantics
> >>     2. The code pattern (brelse then access) is error-prone
> >>     3. smatch warnings clutter output for real issues
> >>>> Fix by moving the brelse(bh) call to the end of the function,
> >>>> after all accesses to bh have been completed.
> >>>>
> >>>> Fixes: e93b100931a4 ("GFS2: Fix slab memory leak in gfs2_bufdata")
> >>>> Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> >>>> ---
> >>>>    fs/gfs2/log.c | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> >>>> index 78bba8cc10b8..a92c84146de9 100644
> >>>> --- a/fs/gfs2/log.c
> >>>> +++ b/fs/gfs2/log.c
> >>>> @@ -1038,7 +1038,6 @@ void gfs2_remove_from_journal(struct buffer_head *bh, int meta)
> >>>>                       set_bit(TR_TOUCHED, &tr->tr_flags);
> >>>>               }
> >>>>               was_pinned = 1;
> >>>> -            brelse(bh);
> >>>>       }
> >>>>       if (bd) {
> >>>>               if (bd->bd_tr) {
> >>>> @@ -1056,6 +1055,8 @@ void gfs2_remove_from_journal(struct buffer_head *bh, int meta)
> >>>>       }
> >>>>       clear_buffer_dirty(bh);
> >>>>       clear_buffer_uptodate(bh);
> >>>> +    if (was_pinned)
> >>>> +            brelse(bh);
> >>>>    }
> >>>>
> >>>>    /**
>