[PATCH v2 0/3] xfs: fix NULL deref in log recovery reorder

[PATCH v2 0/3] xfs: fix NULL deref in log recovery reorder

[Weiming Shi]

A crafted XFS image can commit a transaction whose only item is a bare
transaction header (ri_cnt == 0, ri_buf == NULL).
xlog_recover_reorder_trans() then runs ITEM_TYPE() on it and dereferences
the NULL ri_buf, faulting the kernel at mount time.

v1 was a single patch that added the check but kept an ASSERT(0) on the
path and duplicated the sort_list splice.  Per Christoph's review, v2
splits it:

  1: drop the existing ASSERT(0) on an unrecognized item type
  2: move the sort_list splice out of the loop (no functional change)
  3: add the check that rejects a committed item with no regions

Patch 3 also notes that, since the log is CRC-checked, this is a crafted
image rather than media corruption.

Tested on xfs-7.2-fixes with KASAN: the unpatched kernel oopses in
xlog_recover_reorder_trans; the patched kernel fails the mount with
-EFSCORRUPTED and no splat.

Weiming Shi (3):
  xfs: drop ASSERT(0) on unrecognized log item type
  xfs: splice unsorted log items back to the transaction after the loop
  xfs: fail recovery on a committed log item with no regions

 fs/xfs/xfs_log_recover.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] xfs: drop ASSERT(0) on unrecognized log item type

[Weiming Shi]

The item type passed to ITEM_TYPE() comes from the on-disk log, so a
fuzzed or crafted image can reach the "unrecognized type" path in
xlog_recover_reorder_trans() and trip its ASSERT(0) on a
CONFIG_XFS_DEBUG kernel.  The -EFSCORRUPTED return handles it fine; drop
the assert.

Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 fs/xfs/xfs_log_recover.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 5f984bf5698a..a1b373c68f0e 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1912,7 +1912,6 @@ xlog_recover_reorder_trans(
 			xfs_warn(log->l_mp,
 				"%s: unrecognized type of log operation (%d)",
 				__func__, ITEM_TYPE(item));
-			ASSERT(0);
 			/*
 			 * return the remaining items back to the transaction
 			 * item list so they can be freed in caller.
-- 
2.43.0

[PATCH v2 2/3] xfs: splice unsorted log items back to the transaction after the loop

[Weiming Shi]

On error, xlog_recover_reorder_trans() splices the leftover sort_list
items back to trans->r_itemq inside the loop before breaking out.  The
loop tail already splices the per-fate lists back, so do sort_list there
too, guarded by the assert that used to sit after the loop.

No functional change.  It drops the duplicated splice so the next patch
can add another error case without repeating it.

Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 fs/xfs/xfs_log_recover.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index a1b373c68f0e..5347f6a5ec42 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1912,12 +1912,6 @@ xlog_recover_reorder_trans(
 			xfs_warn(log->l_mp,
 				"%s: unrecognized type of log operation (%d)",
 				__func__, ITEM_TYPE(item));
-			/*
-			 * return the remaining items back to the transaction
-			 * item list so they can be freed in caller.
-			 */
-			if (!list_empty(&sort_list))
-				list_splice_init(&sort_list, &trans->r_itemq);
 			error = -EFSCORRUPTED;
 			break;
 		}
@@ -1945,7 +1939,15 @@ xlog_recover_reorder_trans(
 		}
 	}
 
-	ASSERT(list_empty(&sort_list));
+	/*
+	 * Return the remaining items back to the transaction item list so they
+	 * can be freed in caller.  This should only happen when we encountered
+	 * an error.
+	 */
+	if (!list_empty(&sort_list)) {
+		ASSERT(error);
+		list_splice_init(&sort_list, &trans->r_itemq);
+	}
 	if (!list_empty(&buffer_list))
 		list_splice(&buffer_list, &trans->r_itemq);
 	if (!list_empty(&item_list))
-- 
2.43.0

[PATCH v2 3/3] xfs: fail recovery on a committed log item with no regions

[Weiming Shi]

If the first op of a transaction is a bare transaction header
(len == sizeof(struct xfs_trans_header)), xlog_recover_add_to_trans()
adds a recovery item but no region, leaving it on r_itemq with
ri_cnt == 0 and ri_buf == NULL.

If a commit op follows, xlog_recover_reorder_trans() calls ITEM_TYPE()
on the item, which reads *(unsigned short *)item->ri_buf[0].iov_base and
faults on the NULL ri_buf.

The log is CRC-checked, so this op sequence comes from a crafted image,
not media corruption.  It faults the kernel when such an image is
mounted:

 KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
 RIP: 0010:xlog_recover_reorder_trans (fs/xfs/xfs_log_recover.c:1836)
  xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2043)
  xlog_recover_process_data (fs/xfs/xfs_log_recover.c:2501)
  xlog_do_recovery_pass (fs/xfs/xfs_log_recover.c:3244)
  xlog_recover (fs/xfs/xfs_log_recover.c:3493)
  xfs_log_mount (fs/xfs/xfs_log.c:618)
  xfs_mountfs (fs/xfs/xfs_mount.c:1034)
  xfs_fs_fill_super (fs/xfs/xfs_super.c:1938)
  vfs_get_tree (fs/super.c:1695)
  path_mount (fs/namespace.c:4161)
  __x64_sys_mount (fs/namespace.c:4367)

A committed item always carries its format descriptor in ri_buf[0], so
one with no regions is invalid.  Reject it with -EFSCORRUPTED, like the
unrecognized item type below.

Fixes: 89cebc847729 ("xfs: validate transaction header length on log recovery")
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 fs/xfs/xfs_log_recover.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 5347f6a5ec42..461e847c32a2 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1907,6 +1907,15 @@ xlog_recover_reorder_trans(
 	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
 		enum xlog_recover_reorder	fate = XLOG_REORDER_ITEM_LIST;
 
+		/* a committed item with no regions has a NULL ri_buf[0] */
+		if (!item->ri_cnt || !item->ri_buf) {
+			xfs_warn(log->l_mp,
+				"%s: committed log item has no regions",
+				__func__);
+			error = -EFSCORRUPTED;
+			break;
+		}
+
 		item->ri_ops = xlog_find_item_ops(item);
 		if (!item->ri_ops) {
 			xfs_warn(log->l_mp,
-- 
2.43.0

Re: [PATCH v2 1/3] xfs: drop ASSERT(0) on unrecognized log item type

[Darrick J. Wong]

On Wed, Jul 01, 2026 at 08:38:31AM -0700, Weiming Shi wrote:
> The item type passed to ITEM_TYPE() comes from the on-disk log, so a
> fuzzed or crafted image can reach the "unrecognized type" path in
> xlog_recover_reorder_trans() and trip its ASSERT(0) on a
> CONFIG_XFS_DEBUG kernel.  The -EFSCORRUPTED return handles it fine; drop
> the assert.
> 
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>

<nod> this is a validation of ondisk artifacts, so no assertions

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log_recover.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 5f984bf5698a..a1b373c68f0e 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1912,7 +1912,6 @@ xlog_recover_reorder_trans(
>  			xfs_warn(log->l_mp,
>  				"%s: unrecognized type of log operation (%d)",
>  				__func__, ITEM_TYPE(item));
> -			ASSERT(0);
>  			/*
>  			 * return the remaining items back to the transaction
>  			 * item list so they can be freed in caller.
> -- 
> 2.43.0
> 
> 

Re: [PATCH v2 2/3] xfs: splice unsorted log items back to the transaction after the loop

[Darrick J. Wong]

On Wed, Jul 01, 2026 at 08:38:32AM -0700, Weiming Shi wrote:
> On error, xlog_recover_reorder_trans() splices the leftover sort_list
> items back to trans->r_itemq inside the loop before breaking out.  The
> loop tail already splices the per-fate lists back, so do sort_list there
> too, guarded by the assert that used to sit after the loop.
> 
> No functional change.  It drops the duplicated splice so the next patch
> can add another error case without repeating it.
> 
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
>  fs/xfs/xfs_log_recover.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index a1b373c68f0e..5347f6a5ec42 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1912,12 +1912,6 @@ xlog_recover_reorder_trans(
>  			xfs_warn(log->l_mp,
>  				"%s: unrecognized type of log operation (%d)",
>  				__func__, ITEM_TYPE(item));
> -			/*
> -			 * return the remaining items back to the transaction
> -			 * item list so they can be freed in caller.
> -			 */
> -			if (!list_empty(&sort_list))
> -				list_splice_init(&sort_list, &trans->r_itemq);
>  			error = -EFSCORRUPTED;
>  			break;
>  		}
> @@ -1945,7 +1939,15 @@ xlog_recover_reorder_trans(
>  		}
>  	}
>  
> -	ASSERT(list_empty(&sort_list));
> +	/*
> +	 * Return the remaining items back to the transaction item list so they
> +	 * can be freed in caller.  This should only happen when we encountered

s/encountered/encounter/

(same verb tense across a sentence)

> +	 * an error.
> +	 */
> +	if (!list_empty(&sort_list)) {
> +		ASSERT(error);

This otherwise looks fine to me.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +		list_splice_init(&sort_list, &trans->r_itemq);
> +	}
>  	if (!list_empty(&buffer_list))
>  		list_splice(&buffer_list, &trans->r_itemq);
>  	if (!list_empty(&item_list))
> -- 
> 2.43.0
> 
> 

Re: [PATCH v2 3/3] xfs: fail recovery on a committed log item with no regions

[Darrick J. Wong]

On Wed, Jul 01, 2026 at 08:38:33AM -0700, Weiming Shi wrote:
> If the first op of a transaction is a bare transaction header
> (len == sizeof(struct xfs_trans_header)), xlog_recover_add_to_trans()
> adds a recovery item but no region, leaving it on r_itemq with
> ri_cnt == 0 and ri_buf == NULL.
> 
> If a commit op follows, xlog_recover_reorder_trans() calls ITEM_TYPE()
> on the item, which reads *(unsigned short *)item->ri_buf[0].iov_base and
> faults on the NULL ri_buf.

How do we escape xlog_recover_add_to_trans with item->ri_cnt==0 when
there's a bare transaction header?  Is this the "!len" case at the top
of xlog_recover_add_to_trans?

Why don't we abort log recovery right then and there?  Are we hoping
that a subsequent "continue" section will supply some items, and that's
why we can only check ri_cnt/ri_buf if the ondisk transaction is
committed?

> The log is CRC-checked, so this op sequence comes from a crafted image,
> not media corruption.  It faults the kernel when such an image is
> mounted:
> 
>  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
>  RIP: 0010:xlog_recover_reorder_trans (fs/xfs/xfs_log_recover.c:1836)
>   xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2043)
>   xlog_recover_process_data (fs/xfs/xfs_log_recover.c:2501)
>   xlog_do_recovery_pass (fs/xfs/xfs_log_recover.c:3244)
>   xlog_recover (fs/xfs/xfs_log_recover.c:3493)
>   xfs_log_mount (fs/xfs/xfs_log.c:618)
>   xfs_mountfs (fs/xfs/xfs_mount.c:1034)
>   xfs_fs_fill_super (fs/xfs/xfs_super.c:1938)
>   vfs_get_tree (fs/super.c:1695)
>   path_mount (fs/namespace.c:4161)
>   __x64_sys_mount (fs/namespace.c:4367)
> 
> A committed item always carries its format descriptor in ri_buf[0], so
> one with no regions is invalid.  Reject it with -EFSCORRUPTED, like the
> unrecognized item type below.
> 
> Fixes: 89cebc847729 ("xfs: validate transaction header length on log recovery")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>

Cc: <stable@vger.kernel.org> # v4.3

I agree that a totally empty log item is a sign of corruption, or at
least something going seriously wrong.  Can the runtime log code ever be
tricked into emitting a bare transaction header?

--D

> ---
>  fs/xfs/xfs_log_recover.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 5347f6a5ec42..461e847c32a2 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1907,6 +1907,15 @@ xlog_recover_reorder_trans(
>  	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
>  		enum xlog_recover_reorder	fate = XLOG_REORDER_ITEM_LIST;
>  
> +		/* a committed item with no regions has a NULL ri_buf[0] */
> +		if (!item->ri_cnt || !item->ri_buf) {
> +			xfs_warn(log->l_mp,
> +				"%s: committed log item has no regions",
> +				__func__);
> +			error = -EFSCORRUPTED;
> +			break;
> +		}
> +
>  		item->ri_ops = xlog_find_item_ops(item);
>  		if (!item->ri_ops) {
>  			xfs_warn(log->l_mp,
> -- 
> 2.43.0
> 
> 

Re: [PATCH v2 1/3] xfs: drop ASSERT(0) on unrecognized log item type

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/3] xfs: splice unsorted log items back to the transaction after the loop

[Christoph Hellwig]

On Wed, Jul 01, 2026 at 02:43:54PM -0700, Darrick J. Wong wrote:
> > +	/*
> > +	 * Return the remaining items back to the transaction item list so they
> > +	 * can be freed in caller.  This should only happen when we encountered
> 
> s/encountered/encounter/
> 
> (same verb tense across a sentence)

That might have been my fault :)

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/3] xfs: fail recovery on a committed log item with no regions

[Christoph Hellwig]

On Wed, Jul 01, 2026 at 03:08:38PM -0700, Darrick J. Wong wrote:
> I agree that a totally empty log item is a sign of corruption, or at
> least something going seriously wrong.  Can the runtime log code ever be
> tricked into emitting a bare transaction header?

I don't think so.  This looks like an automated search for bugs in the
parser.  And while I'm happy about tightening up all parsing, I still
think this should be stated in the commit log much more clearly.

Re: [PATCH v2 3/3] xfs: fail recovery on a committed log item with no regions

[Darrick J. Wong]

On Thu, Jul 02, 2026 at 03:56:51AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 01, 2026 at 03:08:38PM -0700, Darrick J. Wong wrote:
> > I agree that a totally empty log item is a sign of corruption, or at
> > least something going seriously wrong.  Can the runtime log code ever be
> > tricked into emitting a bare transaction header?
> 
> I don't think so.  This looks like an automated search for bugs in the
> parser.  And while I'm happy about tightening up all parsing, I still
> think this should be stated in the commit log much more clearly.

Yes, it could at least state whether this is a code change because the
author hit a crash in practice vs. running some tool that pointed out a
gap and emitted a reproducer.

--D

Re: [PATCH v2 3/3] xfs: fail recovery on a committed log item with no regions

[Weiming Shi]

Darrick J. Wong <djwong@kernel.org> 于2026年7月2日周四 23:32写道：
>
> On Thu, Jul 02, 2026 at 03:56:51AM -0700, Christoph Hellwig wrote:
> > On Wed, Jul 01, 2026 at 03:08:38PM -0700, Darrick J. Wong wrote:
> > > I agree that a totally empty log item is a sign of corruption, or at
> > > least something going seriously wrong.  Can the runtime log code ever be
> > > tricked into emitting a bare transaction header?
> >
> > I don't think so.  This looks like an automated search for bugs in the
> > parser.  And while I'm happy about tightening up all parsing, I still
> > think this should be stated in the commit log much more clearly.
>
> Yes, it could at least state whether this is a code change because the
> author hit a crash in practice vs. running some tool that pointed out a
> gap and emitted a reproducer.
>
> --D

Hi,

It came from an AI-assisted code audit of the log recovery parser. I'll
state that in the v3 changelog.

Best,
Weiming Shi