[PATCH v3 0/2] xfs: fixes and clean up for attr item

[PATCH v3 0/2] xfs: fixes and clean up for attr item

[Long Li]

This patch set fixes and cleans up several minor issues in the attr item.

V2->v3:
- Patch1 is discarded, as Darrick has already fixed it.
- Update the comments and add the reviewed tag

V1->V2:
- Fix ip is not initialized issuse in patch-1
- Add the reproduction script in patch-2.
- Delete only the redundant code, and leave the rest unchanged in patch-3.


Long Li (2):
  xfs: fix ri_total validation in xlog_recover_attri_commit_pass2
  xfs: remove redundant validation in xlog_recover_attri_commit_pass2

 fs/xfs/xfs_attr_item.c | 50 ++----------------------------------------
 1 file changed, 2 insertions(+), 48 deletions(-)

-- 
2.39.2

[PATCH v3 1/2] xfs: fix ri_total validation in xlog_recover_attri_commit_pass2

[Long Li]

The ri_total checks for SET/REPLACE operations are hardcoded to 3,
but xfs_attri_item_size() only emits a value iovec when value_len > 0,
so ri_total is 2 when value_len == 0.

For PPTR_SET/PPTR_REMOVE/PPTR_REPLACE, value_len is validated by
xfs_attri_validate() to be exactly sizeof(struct xfs_parent_rec) and
is never zero, so their hardcoded checks remain correct.

This problem may cause log recovery failures. The following script can be
used to reproduce the problem:

 #!/bin/bash
 mkfs.xfs -f /dev/sda
 mount /dev/sda /mnt/test/
 touch /mnt/test/file
 for i in {1..200}; do
         attr -s "user.attr_$i" -V "value_$i" /mnt/test/file > /dev/null
 done
 echo 1 > /sys/fs/xfs/debug/larp
 echo 1 > /sys/fs/xfs/sda/errortag/larp
 attr -s "user.zero" -V "" /mnt/test/file
 echo 0 > /sys/fs/xfs/sda/errortag/larp
 umount /mnt/test
 mount /dev/sda /mnt/test/  # mount failed

Fix this by deriving the expected count dynamically as "2 + !!value_len"
for SET/REPLACE operations.

Cc: <stable@vger.kernel.org> # v6.9
Fixes: ad206ae50eca ("xfs: check opcode and iovec count match in xlog_recover_attri_commit_pass2")
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_attr_item.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 354472bf45f1..83d09635b200 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -1047,8 +1047,8 @@ xlog_recover_attri_commit_pass2(
 		break;
 	case XFS_ATTRI_OP_FLAGS_SET:
 	case XFS_ATTRI_OP_FLAGS_REPLACE:
-		/* Log item, attr name, attr value */
-		if (item->ri_total != 3) {
+		/* Log item, attr name, optional attr value */
+		if (item->ri_total != 2 + !!attri_formatp->alfi_value_len) {
 			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
 					     attri_formatp, len);
 			return -EFSCORRUPTED;
-- 
2.39.2

Re: [PATCH v3 1/2] xfs: fix ri_total validation in xlog_recover_attri_commit_pass2

[Christoph Hellwig]

On Fri, Mar 20, 2026 at 10:11:29AM +0800, Long Li wrote:
> This problem may cause log recovery failures. The following script can be
> used to reproduce the problem:

Can you add this to xfstests, please?

Otherwise looks good:

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

Re: [PATCH v3 1/2] xfs: fix ri_total validation in xlog_recover_attri_commit_pass2

[Long Li]

On Sun, Mar 22, 2026 at 11:15:02PM -0700, Christoph Hellwig wrote:
> On Fri, Mar 20, 2026 at 10:11:29AM +0800, Long Li wrote:
> > This problem may cause log recovery failures. The following script can be
> > used to reproduce the problem:
> 
> Can you add this to xfstests, please?

Okay.

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

Thanks for your review.

Long Li

[PATCH v3 2/2] xfs: remove redundant validation in xlog_recover_attri_commit_pass2

[Long Li]

Remove the redundant post-parse validation switch. By the time that
block is reached, xfs_attri_validate() has already guaranteed all name
lengths are non-zero via xfs_attri_validate_namelen(), and
xfs_attri_validate_name_iovec() has already returned -EFSCORRUPTED for
NULL names. For the REMOVE case, attr_value and value_len are
structurally guaranteed to be NULL/zero because the parsing loop only
populates them when value_len != 0. All checks in that switch are
therefore dead code.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_attr_item.c | 46 ------------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 83d09635b200..82324f42537b 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -1132,52 +1132,6 @@ xlog_recover_attri_commit_pass2(
 		return -EFSCORRUPTED;
 	}
 
-	switch (op) {
-	case XFS_ATTRI_OP_FLAGS_REMOVE:
-		/* Regular remove operations operate only on names. */
-		if (attr_value != NULL || value_len != 0) {
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-					     attri_formatp, len);
-			return -EFSCORRUPTED;
-		}
-		fallthrough;
-	case XFS_ATTRI_OP_FLAGS_PPTR_REMOVE:
-	case XFS_ATTRI_OP_FLAGS_PPTR_SET:
-	case XFS_ATTRI_OP_FLAGS_SET:
-	case XFS_ATTRI_OP_FLAGS_REPLACE:
-		/*
-		 * Regular xattr set/remove/replace operations require a name
-		 * and do not take a newname.  Values are optional for set and
-		 * replace.
-		 *
-		 * Name-value set/remove operations must have a name, do not
-		 * take a newname, and can take a value.
-		 */
-		if (attr_name == NULL || name_len == 0) {
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-					     attri_formatp, len);
-			return -EFSCORRUPTED;
-		}
-		break;
-	case XFS_ATTRI_OP_FLAGS_PPTR_REPLACE:
-		/*
-		 * Name-value replace operations require the caller to
-		 * specify the old and new names and values explicitly.
-		 * Values are optional.
-		 */
-		if (attr_name == NULL || name_len == 0) {
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-					     attri_formatp, len);
-			return -EFSCORRUPTED;
-		}
-		if (attr_new_name == NULL || new_name_len == 0) {
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-					     attri_formatp, len);
-			return -EFSCORRUPTED;
-		}
-		break;
-	}
-
 	/*
 	 * Memory alloc failure will cause replay to abort.  We attach the
 	 * name/value buffer to the recovered incore log item and drop our
-- 
2.39.2

Re: [PATCH v3 2/2] xfs: remove redundant validation in xlog_recover_attri_commit_pass2

[Christoph Hellwig]

On Fri, Mar 20, 2026 at 10:11:30AM +0800, Long Li wrote:
> Remove the redundant post-parse validation switch. By the time that
> block is reached, xfs_attri_validate() has already guaranteed all name
> lengths are non-zero via xfs_attri_validate_namelen(), and
> xfs_attri_validate_name_iovec() has already returned -EFSCORRUPTED for
> NULL names. For the REMOVE case, attr_value and value_len are
> structurally guaranteed to be NULL/zero because the parsing loop only
> populates them when value_len != 0. All checks in that switch are
> therefore dead code.

Looks good:

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

Re: [PATCH v3 0/2] xfs: fixes and clean up for attr item

[Carlos Maiolino]

On Fri, 20 Mar 2026 10:11:28 +0800, Long Li wrote:
> This patch set fixes and cleans up several minor issues in the attr item.
> 
> V2->v3:
> - Patch1 is discarded, as Darrick has already fixed it.
> - Update the comments and add the reviewed tag
> 
> V1->V2:
> - Fix ip is not initialized issuse in patch-1
> - Add the reproduction script in patch-2.
> - Delete only the redundant code, and leave the rest unchanged in patch-3.
> 
> [...]

Applied to for-next, thanks!

[1/2] xfs: fix ri_total validation in xlog_recover_attri_commit_pass2
      commit: d72f2084e30966097c8eae762e31986a33c3c0ae
[2/2] xfs: remove redundant validation in xlog_recover_attri_commit_pass2
      commit: c6c56ff975f046be25f527231a239e37920aca5e

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>