[PATCH] gfs2: keep glocks alive while AIL entries exist

[PATCH] gfs2: keep glocks alive while AIL entries exist

[Eduard Zateev]

A buffer on a glock AIL list can outlive the glock reference held by its
inode. During withdrawal, gfs2_ail_drain() later removes the buffer from
gl_ail_list, which dereferences the freed glock.

Hold one glock reference while its AIL count is nonzero. Acquire the
reference for the first AIL entry and release it asynchronously when the
last entry is removed, since AIL removal runs under sd_ail_lock.

Build-tested: x86-64 GFS2 objects with W=1; arm64 GFS2 objects
also compile.
The syzbot report has no usable reproducer on current HEAD, so a runtime
test was not possible.

Fixes: 864174f97647 ("gfs2: Don't cache unreferenced glocks")
Reported-by: syzbot+ab91b239b20ae188b9cb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ab91b239b20ae188b9cb
Assisted-by: LLM
Signed-off-by: Eduard Zateev <hackerowskiy@gmail.com>
---
 fs/gfs2/log.c  | 5 ++++-
 fs/gfs2/lops.c | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index b55daf1e638..aa5f2a9a94f 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -74,10 +74,13 @@ unsigned int gfs2_struct2blk(struct gfs2_sbd *sdp, unsigned int nstruct)
 
 static void gfs2_remove_from_ail(struct gfs2_bufdata *bd)
 {
+	struct gfs2_glock *gl = bd->bd_gl;
+
 	bd->bd_tr = NULL;
 	list_del_init(&bd->bd_ail_st_list);
 	list_del_init(&bd->bd_ail_gl_list);
-	atomic_dec(&bd->bd_gl->gl_ail_count);
+	if (atomic_dec_and_test(&gl->gl_ail_count))
+		gfs2_glock_put_async(gl);
 	brelse(bd->bd_bh);
 }
 
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 341f5bb844b..c4e63fb375b 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -120,8 +120,9 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct buffer_head *bh,
 		brelse(bh);
 	} else {
 		struct gfs2_glock *gl = bd->bd_gl;
+		if (atomic_inc_return(&gl->gl_ail_count) == 1)
+			gfs2_glock_hold(gl);
 		list_add(&bd->bd_ail_gl_list, &gl->gl_ail_list);
-		atomic_inc(&gl->gl_ail_count);
 	}
 	bd->bd_tr = tr;
 	list_add(&bd->bd_ail_st_list, &tr->tr_ail1_list);
-- 
2.55.0

Re: [PATCH] gfs2: keep glocks alive while AIL entries exist

[Andreas Gruenbacher]

Eduard,

On Fri, Oct 23, 2026 at 12:19 PM Eduard Zateev <hackerowskiy@gmail.com> wrote:
> A buffer on a glock AIL list can outlive the glock reference held by its
> inode. During withdrawal, gfs2_ail_drain() later removes the buffer from
> gl_ail_list, which dereferences the freed glock.
>
> Hold one glock reference while its AIL count is nonzero. Acquire the
> reference for the first AIL entry and release it asynchronously when the
> last entry is removed, since AIL removal runs under sd_ail_lock.
>
> Build-tested: x86-64 GFS2 objects with W=1; arm64 GFS2 objects
> also compile.
> The syzbot report has no usable reproducer on current HEAD, so a runtime
> test was not possible.

thank you for this patch. I agree with the solution, but I'll merge it
with a better description.

> Fixes: 864174f97647 ("gfs2: Don't cache unreferenced glocks")
> Reported-by: syzbot+ab91b239b20ae188b9cb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ab91b239b20ae188b9cb
> Assisted-by: LLM

Nit: Assisted-by tags are now discouraged.

> Signed-off-by: Eduard Zateev <hackerowskiy@gmail.com>
> ---
>  fs/gfs2/log.c  | 5 ++++-
>  fs/gfs2/lops.c | 3 ++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index b55daf1e638..aa5f2a9a94f 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -74,10 +74,13 @@ unsigned int gfs2_struct2blk(struct gfs2_sbd *sdp, unsigned int nstruct)
>
>  static void gfs2_remove_from_ail(struct gfs2_bufdata *bd)
>  {
> +       struct gfs2_glock *gl = bd->bd_gl;
> +
>         bd->bd_tr = NULL;
>         list_del_init(&bd->bd_ail_st_list);
>         list_del_init(&bd->bd_ail_gl_list);
> -       atomic_dec(&bd->bd_gl->gl_ail_count);
> +       if (atomic_dec_and_test(&gl->gl_ail_count))
> +               gfs2_glock_put_async(gl);
>         brelse(bd->bd_bh);
>  }
>
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 341f5bb844b..c4e63fb375b 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -120,8 +120,9 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct buffer_head *bh,
>                 brelse(bh);
>         } else {
>                 struct gfs2_glock *gl = bd->bd_gl;
> +               if (atomic_inc_return(&gl->gl_ail_count) == 1)
> +                       gfs2_glock_hold(gl);
>                 list_add(&bd->bd_ail_gl_list, &gl->gl_ail_list);
> -               atomic_inc(&gl->gl_ail_count);
>         }
>         bd->bd_tr = tr;
>         list_add(&bd->bd_ail_st_list, &tr->tr_ail1_list);
> --
> 2.55.0
>

[PATCH] gfs2: keep glocks alive while AIL entries exist

[Andreas Gruenbacher]

Before this patch, gfs2_add_revoke() took an extra glock reference for
the first revoke of a glock, and gfs2_glock_remove_revoke() released
that reference when the last revoke of a glock was removed.  This
ensures that the glock stays alive while the sd_log_revokes list
contains revokes for this glock, but it only helps if the glock is still
alive when gfs2_add_revoke() is called.

Commit 657e5af4fe0b ("gfs2: harden gfs2_glock_hold") has revealed that
there are cases in which this is not the case.  In addition, commit
864174f97647 ("gfs2: Don't cache unreferenced glocks") started freeing
glocks more aggressively, so an actual use-after-free is now a lot more
likely.

To fix that, take an additional glock reference when the first bufdata
object for a glock is moved onto the AIL, and drop that reference when
the last bufdata object of a glock is removed from the AIL.

In gfs2_add_revoke(), gl_revokes is incremented before gl_ail_count is
decremented, which this guarantees that the glock will stay referenced
when a bufdata object is moved from the AIL onto the revokes list.

In gfs2_glock_remove_revoke(), switch from 'atomic_dec_return() == 0' to
the equivalent atomic_dec_and_test() to use the same code as in
gfs2_remove_from_ail().

Except for the patch decription, this patch is basically identical to
the one proposed by Eduard Zateev.

Suggested-by: Eduard Zateev <hackerowskiy@gmail.com>
Fixes: 864174f97647 ("gfs2: Don't cache unreferenced glocks")
Reported-by: syzbot+ab91b239b20ae188b9cb@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ab91b239b20ae188b9cb
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index b55daf1e6381..7a391b50d13b 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -74,10 +74,13 @@ unsigned int gfs2_struct2blk(struct gfs2_sbd *sdp, unsigned int nstruct)
 
 static void gfs2_remove_from_ail(struct gfs2_bufdata *bd)
 {
+	struct gfs2_glock *gl = bd->bd_gl;
+
 	bd->bd_tr = NULL;
 	list_del_init(&bd->bd_ail_st_list);
 	list_del_init(&bd->bd_ail_gl_list);
-	atomic_dec(&bd->bd_gl->gl_ail_count);
+	if (atomic_dec_and_test(&gl->gl_ail_count))
+		gfs2_glock_put_async(gl);
 	brelse(bd->bd_bh);
 }
 
@@ -778,7 +781,7 @@ void gfs2_add_revoke(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)
 
 void gfs2_glock_remove_revoke(struct gfs2_glock *gl)
 {
-	if (atomic_dec_return(&gl->gl_revokes) == 0) {
+	if (atomic_dec_and_test(&gl->gl_revokes)) {
 		clear_bit(GLF_LFLUSH, &gl->gl_flags);
 		gfs2_glock_put_async(gl);
 	}
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 341f5bb844bb..13d8aa1dfea1 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -121,7 +121,8 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct buffer_head *bh,
 	} else {
 		struct gfs2_glock *gl = bd->bd_gl;
 		list_add(&bd->bd_ail_gl_list, &gl->gl_ail_list);
-		atomic_inc(&gl->gl_ail_count);
+		if (atomic_inc_return(&gl->gl_ail_count) == 1)
+			gfs2_glock_hold(gl);
 	}
 	bd->bd_tr = tr;
 	list_add(&bd->bd_ail_st_list, &tr->tr_ail1_list);