[PATCH] xfs: add guard before freeing buffer log item

[PATCH] xfs: add guard before freeing buffer log item

[Marcelo Mendes Spessoto Junior]

A buffer's write completion can race with the CIL walking the same
checkpoint's item list during a forced shutdown's simulated commit
callbacks. Whichever side reaches the item last should be the one to
free it; right now completion frees it unconditionally, so the CIL
walk can end up touching memory that's already gone.

Fixes: d2fe5c4c8d25 ("xfs: rearrange code in xfs_buf_item.c")
Reported-by: syzbot+4e6ee73c0ae4b6e8753f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4e6ee73c0ae4b6e8753f
Tested-by: syzbot+4e6ee73c0ae4b6e8753f@syzkaller.appspotmail.com
Signed-off-by: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>
---
 fs/xfs/xfs_buf_item.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index f4c5be67826e..c86c4387f52b 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1072,6 +1072,9 @@ void
 xfs_buf_item_done(
 	struct xfs_buf		*bp)
 {
+	if (atomic_read(&bp->b_log_item->bli_refcount) != 0)
+		return;
+
 	/*
 	 * If we are forcibly shutting down, this may well be off the AIL
 	 * already. That's because we simulate the log-committed callbacks to
-- 
2.55.0

Re: [PATCH] xfs: add guard before freeing buffer log item

[Marcelo Mendes]

I've missed the original discussion about the issue
(https://lore.kernel.org/all/68a70f15.050a0220.3d78fd.0024.GAE@google.com/T/)
before submitting the fix. I will work on a v2 addressing the root
causes, and this patch can be ignored.

On Fri, Jul 31, 2026 at 3:17 AM Marcelo Mendes Spessoto Junior
<marcelomspessoto@gmail.com> wrote:
>
> A buffer's write completion can race with the CIL walking the same
> checkpoint's item list during a forced shutdown's simulated commit
> callbacks. Whichever side reaches the item last should be the one to
> free it; right now completion frees it unconditionally, so the CIL
> walk can end up touching memory that's already gone.
>
> Fixes: d2fe5c4c8d25 ("xfs: rearrange code in xfs_buf_item.c")
> Reported-by: syzbot+4e6ee73c0ae4b6e8753f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4e6ee73c0ae4b6e8753f
> Tested-by: syzbot+4e6ee73c0ae4b6e8753f@syzkaller.appspotmail.com
> Signed-off-by: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>
> ---
>  fs/xfs/xfs_buf_item.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index f4c5be67826e..c86c4387f52b 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1072,6 +1072,9 @@ void
>  xfs_buf_item_done(
>         struct xfs_buf          *bp)
>  {
> +       if (atomic_read(&bp->b_log_item->bli_refcount) != 0)
> +               return;
> +
>         /*
>          * If we are forcibly shutting down, this may well be off the AIL
>          * already. That's because we simulate the log-committed callbacks to
> --
> 2.55.0
>

Re: [PATCH] xfs: add guard before freeing buffer log item

[Dave Chinner]

On Fri, Jul 31, 2026 at 03:14:48AM -0300, Marcelo Mendes Spessoto Junior wrote:
> A buffer's write completion can race with the CIL walking the same
> checkpoint's item list during a forced shutdown's simulated commit
> callbacks. Whichever side reaches the item last should be the one to
> free it; right now completion frees it unconditionally, so the CIL
> walk can end up touching memory that's already gone.
> 
> Fixes: d2fe5c4c8d25 ("xfs: rearrange code in xfs_buf_item.c")
> Reported-by: syzbot+4e6ee73c0ae4b6e8753f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4e6ee73c0ae4b6e8753f
> Tested-by: syzbot+4e6ee73c0ae4b6e8753f@syzkaller.appspotmail.com
> Signed-off-by: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>
> ---
>  fs/xfs/xfs_buf_item.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index f4c5be67826e..c86c4387f52b 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1072,6 +1072,9 @@ void
>  xfs_buf_item_done(
>  	struct xfs_buf		*bp)
>  {
> +	if (atomic_read(&bp->b_log_item->bli_refcount) != 0)
> +		return;

No.

My conclusion to the original syzbot report was:

| As such, I think that the issue we need to understand here is why a
| write IO completion is being run from read IO context. That's the
| root cause of the UAF, not the commit the bisect landed on....

And if you look at what I posted yesterday about a different hack to
this same code:

https://lore.kernel.org/linux-xfs/amvI_rVUlifIXoZA@dread/

which "fixes" a UAF in the CIL code reported by KASAN under syzbot
conditions. Whilst the syzbot report quoted was different:

https://syzkaller.appspot.com/bug?extid=598a791b31c498b63c6b

The syzbot report indicates exactly the same UAF situation - a write
IO completion being run from a read IO context causing the BLI to be
freed incorrectly and then the CIL access to the BLI triggering a
UAF.  My conclusion to that one is:

| Given that syzbot has only reported this 5 times in 12 hours only on
| a 7.1-rc3 kernel, never before and never since, an external memory
| corruption bug that has since been fixed seems like the most like
| cause here.

All the recent reports of this issue (the cluster in mid May) are
also only from v7.1-rc3 kernels....

So, yeah, people really need to stop trying to hack "fixes" into
xfs_buf_item_done(). The UAF indicates that there has been an
-incorrect free of the item- during IO completion. That's the issue
that needs to be understood and fixed. Fundamentally breaking the
refcounting model of the BLI to silence the downstream UAF that
results from the incorrect free is not an acceptible solution.

-Dave.
-- 
Dave Chinner
dgc@kernel.org