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

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

[Long Li]

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

Long Li (3):
  xfs: fix possible null pointer dereference in xfs_attri_recover_work
  xfs: fix ri_total validation in xlog_recover_attri_commit_pass2
  xfs: simplify iovec validation in xlog_recover_attri_commit_pass2

 fs/xfs/xfs_attr_item.c | 82 ++++++++----------------------------------
 1 file changed, 14 insertions(+), 68 deletions(-)

-- 
2.39.2

[PATCH 1/3] xfs: fix possible null pointer dereference in xfs_attri_recover_work

[Long Li]

When xlog_recover_iget() or xlog_recover_iget_handle() fails, ip is
not guaranteed to be initialized. Calling xfs_irele(ip) unconditionally
in the error path may dereference a null pointer.

Cc: <stable@vger.kernel.org> # v6.9
Fixes: ae673f534a30 ("xfs: record inode generation in xattr update log intent items")
Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_attr_item.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 354472bf45f1..3d3ac8dad519 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -653,7 +653,8 @@ xfs_attri_recover_work(
 		break;
 	}
 	if (error) {
-		xfs_irele(ip);
+		if (ip)
+			xfs_irele(ip);
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
 				sizeof(*attrp));
 		return ERR_PTR(-EFSCORRUPTED);
-- 
2.39.2

Re: [PATCH 1/3] xfs: fix possible null pointer dereference in xfs_attri_recover_work

[Darrick J. Wong]

On Mon, Mar 16, 2026 at 09:24:14AM +0800, Long Li wrote:
> When xlog_recover_iget() or xlog_recover_iget_handle() fails, ip is
> not guaranteed to be initialized. Calling xfs_irele(ip) unconditionally
> in the error path may dereference a null pointer.

Don't you need to fix the variable declaration with a null initializer
too?

--D

> Cc: <stable@vger.kernel.org> # v6.9
> Fixes: ae673f534a30 ("xfs: record inode generation in xattr update log intent items")
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>  fs/xfs/xfs_attr_item.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 354472bf45f1..3d3ac8dad519 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -653,7 +653,8 @@ xfs_attri_recover_work(
>  		break;
>  	}
>  	if (error) {
> -		xfs_irele(ip);
> +		if (ip)
> +			xfs_irele(ip);
>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
>  				sizeof(*attrp));
>  		return ERR_PTR(-EFSCORRUPTED);
> -- 
> 2.39.2
> 
> 

Re: [PATCH 1/3] xfs: fix possible null pointer dereference in xfs_attri_recover_work

[Long Li]

On Mon, Mar 16, 2026 at 03:33:57PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 16, 2026 at 09:24:14AM +0800, Long Li wrote:
> > When xlog_recover_iget() or xlog_recover_iget_handle() fails, ip is
> > not guaranteed to be initialized. Calling xfs_irele(ip) unconditionally
> > in the error path may dereference a null pointer.
> 
> Don't you need to fix the variable declaration with a null initializer
> too?
> 
> --D

Sorry, I didn't notice, it will be update. :)

Thanks,
Long Li

> 
> > Cc: <stable@vger.kernel.org> # v6.9
> > Fixes: ae673f534a30 ("xfs: record inode generation in xattr update log intent items")
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >  fs/xfs/xfs_attr_item.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 354472bf45f1..3d3ac8dad519 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -653,7 +653,8 @@ xfs_attri_recover_work(
> >  		break;
> >  	}
> >  	if (error) {
> > -		xfs_irele(ip);
> > +		if (ip)
> > +			xfs_irele(ip);
> >  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
> >  				sizeof(*attrp));
> >  		return ERR_PTR(-EFSCORRUPTED);
> > -- 
> > 2.39.2
> > 
> > 
> 

[PATCH 2/3] 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.

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")
Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_attr_item.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 3d3ac8dad519..21da995ba4e7 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -1049,7 +1049,7 @@ xlog_recover_attri_commit_pass2(
 	case XFS_ATTRI_OP_FLAGS_SET:
 	case XFS_ATTRI_OP_FLAGS_REPLACE:
 		/* Log item, attr name, attr value */
-		if (item->ri_total != 3) {
+		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 2/3] xfs: fix ri_total validation in xlog_recover_attri_commit_pass2

[Darrick J. Wong]

On Mon, Mar 16, 2026 at 09:24:15AM +0800, Long Li wrote:
> 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.

When can you have a set/replace operation with no value?  Is this the
weird case where you're trying to set an attr to zero-length buffer?

--D

> 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.
> 
> 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")
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>  fs/xfs/xfs_attr_item.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 3d3ac8dad519..21da995ba4e7 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -1049,7 +1049,7 @@ xlog_recover_attri_commit_pass2(
>  	case XFS_ATTRI_OP_FLAGS_SET:
>  	case XFS_ATTRI_OP_FLAGS_REPLACE:
>  		/* Log item, attr name, attr value */
> -		if (item->ri_total != 3) {
> +		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 2/3] xfs: fix ri_total validation in xlog_recover_attri_commit_pass2

[Long Li]

On Mon, Mar 16, 2026 at 03:39:21PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 16, 2026 at 09:24:15AM +0800, Long Li wrote:
> > 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.
> 
> When can you have a set/replace operation with no value?  Is this the
> weird case where you're trying to set an attr to zero-length buffer?
> 
> --D

Yes, the issue can be reproduced using the following command:

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_000000000000000_$i" /mnt/test/file > /dev/null
done
echo 1 > /sys/fs/xfs/debug/larp
mount /dev/sda /mnt/test/
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/


Damage is reported when mounting again:

[45643.683467] XFS (sda): Internal error xlog_recover_attri_commit_pass2 at line 1052 of file fs/xfs/xfs_attr_item.c.  Caller xlog_recover_items_pass2+0xe1/0x290
[45643.684743] CPU: 2 UID: 0 PID: 2328 Comm: mount Tainted: G    B               7.0.0-rc3-next-20260312-gcb0d985e40d9-dirty #352 PREEMPT(full)
[45643.684749] Tainted: [B]=BAD_PAGE
[45643.684751] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014
[45643.684753] Call Trace:
[45643.684755]  <TASK>
[45643.684757]  dump_stack_lvl+0x53/0x70
[45643.684764]  xfs_corruption_error+0xf9/0x150
[45643.684770]  ? xlog_recover_items_pass2+0xe1/0x290
[45643.684774]  xlog_recover_attri_commit_pass2+0x866/0x1380
[45643.684778]  ? xlog_recover_items_pass2+0xe1/0x290
[45643.684782]  xlog_recover_items_pass2+0xe1/0x290
[45643.684786]  xlog_recover_commit_trans+0x6ca/0xa30
[45643.684791]  ? __pfx_xlog_recover_commit_trans+0x10/0x10
[45643.684794]  ? xlog_recover_process_ophdr+0x18f/0x400
[45643.684798]  xlog_recovery_process_trans+0x11b/0x150
[45643.684802]  xlog_recover_process_data+0x185/0x360
[45643.684806]  xlog_do_recovery_pass+0x810/0xbf0
[45643.684811]  ? __pfx_xlog_do_recovery_pass+0x10/0x10
[45643.684814]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[45643.684821]  ? kasan_save_track+0x14/0x30
[45643.684826]  ? __kasan_kmalloc+0x8f/0xa0
[45643.684830]  xlog_do_log_recovery+0x66/0xb0
[45643.684833]  xlog_do_recover+0x75/0x3b0
[45643.684837]  xlog_recover+0x25b/0x460
[45643.684841]  ? __pfx_xlog_recover+0x10/0x10
[45643.684845]  xfs_log_mount+0x1c1/0x420
[45643.684850]  xfs_mountfs+0xc9e/0x1c20
[45643.684853]  ? __link_object+0x10f/0x210
[45643.684858]  ? __pfx_xfs_mountfs+0x10/0x10
[45643.684861]  ? xfs_mru_cache_create+0x10a/0x5e0
[45643.684865]  ? __pfx_xfs_fstrm_free_func+0x10/0x10
[45643.684869]  ? xfs_mru_cache_create+0x3b5/0x5e0
[45643.684872]  xfs_fs_fill_super+0xdcd/0x18d0
[45643.684878]  ? __pfx_xfs_fs_fill_super+0x10/0x10
[45643.684882]  get_tree_bdev_flags+0x2ec/0x550
[45643.684888]  ? __pfx_get_tree_bdev_flags+0x10/0x10
[45643.684893]  ? __pfx_vfs_parse_fs_qstr+0x10/0x10
[45643.684898]  vfs_get_tree+0x87/0x2d0
[45643.684903]  fc_mount+0x15/0x1b0
[45643.684908]  path_mount+0x116b/0x1b00
[45643.684912]  ? __pfx_path_mount+0x10/0x10
[45643.684916]  ? kasan_save_track+0x14/0x30
[45643.684919]  ? user_path_at+0x43/0x50
[45643.684922]  ? kmem_cache_free+0x248/0x490
[45643.684928]  __x64_sys_mount+0x210/0x270
[45643.684932]  ? __pfx___x64_sys_mount+0x10/0x10
[45643.684936]  do_syscall_64+0xb4/0x5c0
[45643.684941]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[45643.684945] RIP: 0033:0x7fe945b1123a
[45643.684949] Code: 48 8b 0d 51 dc 2b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1e dc 2b 00 f7 d8 64 89 01 48
[45643.684953] RSP: 002b:00007ffc3102ede8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
[45643.684957] RAX: ffffffffffffffda RBX: 0000559d1d098030 RCX: 00007fe945b1123a
[45643.684960] RDX: 0000559d1d098230 RSI: 0000559d1d09af20 RDI: 0000559d1d098210
[45643.684962] RBP: 0000000000000000 R08: 0000000000000000 R09: 00007ffc3102dce8
[45643.684963] R10: 00000000c0ed0000 R11: 0000000000000246 R12: 0000559d1d098210
[45643.684966] R13: 0000559d1d098230 R14: 0000000000000000 R15: 00007fe947052184

[PATCH 3/3] xfs: simplify iovec validation in xlog_recover_attri_commit_pass2

[Long Li]

Consolidate the per-case ri_total checks into a single comparison by
assigning the expected iovec count to a local variable inside each
switch arm, removing four nearly identical error blocks.

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.

Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_attr_item.c | 79 +++++++-----------------------------------
 1 file changed, 12 insertions(+), 67 deletions(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 21da995ba4e7..32236f5008ec 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -1016,12 +1016,13 @@ xlog_recover_attri_commit_pass2(
 	unsigned int			new_name_len = 0;
 	unsigned int			new_value_len = 0;
 	unsigned int			op, i = 0;
+	unsigned int			expected = 0;
 
 	/* Validate xfs_attri_log_format before the large memory allocation */
 	len = sizeof(struct xfs_attri_log_format);
 	if (item->ri_buf[i].iov_len != len) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-				item->ri_buf[0].iov_base, item->ri_buf[0].iov_len);
+				item->ri_buf[i].iov_base, item->ri_buf[i].iov_len);
 		return -EFSCORRUPTED;
 	}
 
@@ -1038,32 +1039,20 @@ xlog_recover_attri_commit_pass2(
 	case XFS_ATTRI_OP_FLAGS_PPTR_REMOVE:
 	case XFS_ATTRI_OP_FLAGS_PPTR_SET:
 		/* Log item, attr name, attr value */
-		if (item->ri_total != 3) {
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-					     attri_formatp, len);
-			return -EFSCORRUPTED;
-		}
+		expected = 3;
 		name_len = attri_formatp->alfi_name_len;
 		value_len = attri_formatp->alfi_value_len;
 		break;
 	case XFS_ATTRI_OP_FLAGS_SET:
 	case XFS_ATTRI_OP_FLAGS_REPLACE:
 		/* Log item, attr name, 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;
-		}
+		expected = 2 + !!attri_formatp->alfi_value_len;
 		name_len = attri_formatp->alfi_name_len;
 		value_len = attri_formatp->alfi_value_len;
 		break;
 	case XFS_ATTRI_OP_FLAGS_REMOVE:
 		/* Log item, attr name */
-		if (item->ri_total != 2) {
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-					     attri_formatp, len);
-			return -EFSCORRUPTED;
-		}
+		expected = 2;
 		name_len = attri_formatp->alfi_name_len;
 		break;
 	case XFS_ATTRI_OP_FLAGS_PPTR_REPLACE:
@@ -1071,11 +1060,7 @@ xlog_recover_attri_commit_pass2(
 		 * Log item, attr name, new attr name, attr value, new attr
 		 * value
 		 */
-		if (item->ri_total != 5) {
-			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
-					     attri_formatp, len);
-			return -EFSCORRUPTED;
-		}
+		expected = 5;
 		name_len = attri_formatp->alfi_old_name_len;
 		new_name_len = attri_formatp->alfi_new_name_len;
 		new_value_len = value_len = attri_formatp->alfi_value_len;
@@ -1085,6 +1070,12 @@ xlog_recover_attri_commit_pass2(
 				     attri_formatp, len);
 		return -EFSCORRUPTED;
 	}
+
+	if (item->ri_total != expected) {
+		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
+				attri_formatp, len);
+		return -EFSCORRUPTED;
+	}
 	i++;
 
 	/* Validate the attr name */
@@ -1133,52 +1124,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 3/3] xfs: simplify iovec validation in xlog_recover_attri_commit_pass2

[Darrick J. Wong]

On Mon, Mar 16, 2026 at 09:24:16AM +0800, Long Li wrote:
> Consolidate the per-case ri_total checks into a single comparison by
> assigning the expected iovec count to a local variable inside each
> switch arm, removing four nearly identical error blocks.
> 
> 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.
> 
> Signed-off-by: Long Li <leo.lilong@huawei.com>
> ---
>  fs/xfs/xfs_attr_item.c | 79 +++++++-----------------------------------
>  1 file changed, 12 insertions(+), 67 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 21da995ba4e7..32236f5008ec 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -1016,12 +1016,13 @@ xlog_recover_attri_commit_pass2(
>  	unsigned int			new_name_len = 0;
>  	unsigned int			new_value_len = 0;
>  	unsigned int			op, i = 0;
> +	unsigned int			expected = 0;
>  
>  	/* Validate xfs_attri_log_format before the large memory allocation */
>  	len = sizeof(struct xfs_attri_log_format);
>  	if (item->ri_buf[i].iov_len != len) {
>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> -				item->ri_buf[0].iov_base, item->ri_buf[0].iov_len);
> +				item->ri_buf[i].iov_base, item->ri_buf[i].iov_len);
>  		return -EFSCORRUPTED;
>  	}
>  
> @@ -1038,32 +1039,20 @@ xlog_recover_attri_commit_pass2(
>  	case XFS_ATTRI_OP_FLAGS_PPTR_REMOVE:
>  	case XFS_ATTRI_OP_FLAGS_PPTR_SET:
>  		/* Log item, attr name, attr value */
> -		if (item->ri_total != 3) {
> -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> -					     attri_formatp, len);
> -			return -EFSCORRUPTED;
> -		}
> +		expected = 3;
>  		name_len = attri_formatp->alfi_name_len;
>  		value_len = attri_formatp->alfi_value_len;
>  		break;
>  	case XFS_ATTRI_OP_FLAGS_SET:
>  	case XFS_ATTRI_OP_FLAGS_REPLACE:
>  		/* Log item, attr name, 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;
> -		}
> +		expected = 2 + !!attri_formatp->alfi_value_len;
>  		name_len = attri_formatp->alfi_name_len;
>  		value_len = attri_formatp->alfi_value_len;
>  		break;
>  	case XFS_ATTRI_OP_FLAGS_REMOVE:
>  		/* Log item, attr name */
> -		if (item->ri_total != 2) {
> -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> -					     attri_formatp, len);
> -			return -EFSCORRUPTED;
> -		}
> +		expected = 2;
>  		name_len = attri_formatp->alfi_name_len;
>  		break;
>  	case XFS_ATTRI_OP_FLAGS_PPTR_REPLACE:
> @@ -1071,11 +1060,7 @@ xlog_recover_attri_commit_pass2(
>  		 * Log item, attr name, new attr name, attr value, new attr
>  		 * value
>  		 */
> -		if (item->ri_total != 5) {
> -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> -					     attri_formatp, len);
> -			return -EFSCORRUPTED;
> -		}
> +		expected = 5;
>  		name_len = attri_formatp->alfi_old_name_len;
>  		new_name_len = attri_formatp->alfi_new_name_len;
>  		new_value_len = value_len = attri_formatp->alfi_value_len;
> @@ -1085,6 +1070,12 @@ xlog_recover_attri_commit_pass2(
>  				     attri_formatp, len);
>  		return -EFSCORRUPTED;
>  	}
> +
> +	if (item->ri_total != expected) {
> +		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,

The downside to this change is that XFS_CORRUPTION_ERROR prints the
source file and line number, so now anyone looking through the logs
cannot identify a specific line number.  However...

> +				attri_formatp, len);
> +		return -EFSCORRUPTED;
> +	}
>  	i++;
>  
>  	/* Validate the attr name */
> @@ -1133,52 +1124,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;
> -	}

...this part is indeed redundant.

--D

> -
>  	/*
>  	 * 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 3/3] xfs: simplify iovec validation in xlog_recover_attri_commit_pass2

[Long Li]

On Mon, Mar 16, 2026 at 03:46:32PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 16, 2026 at 09:24:16AM +0800, Long Li wrote:
> > Consolidate the per-case ri_total checks into a single comparison by
> > assigning the expected iovec count to a local variable inside each
> > switch arm, removing four nearly identical error blocks.
> > 
> > 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.
> > 
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >  fs/xfs/xfs_attr_item.c | 79 +++++++-----------------------------------
> >  1 file changed, 12 insertions(+), 67 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 21da995ba4e7..32236f5008ec 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -1016,12 +1016,13 @@ xlog_recover_attri_commit_pass2(
> >  	unsigned int			new_name_len = 0;
> >  	unsigned int			new_value_len = 0;
> >  	unsigned int			op, i = 0;
> > +	unsigned int			expected = 0;
> >  
> >  	/* Validate xfs_attri_log_format before the large memory allocation */
> >  	len = sizeof(struct xfs_attri_log_format);
> >  	if (item->ri_buf[i].iov_len != len) {
> >  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> > -				item->ri_buf[0].iov_base, item->ri_buf[0].iov_len);
> > +				item->ri_buf[i].iov_base, item->ri_buf[i].iov_len);
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> > @@ -1038,32 +1039,20 @@ xlog_recover_attri_commit_pass2(
> >  	case XFS_ATTRI_OP_FLAGS_PPTR_REMOVE:
> >  	case XFS_ATTRI_OP_FLAGS_PPTR_SET:
> >  		/* Log item, attr name, attr value */
> > -		if (item->ri_total != 3) {
> > -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> > -					     attri_formatp, len);
> > -			return -EFSCORRUPTED;
> > -		}
> > +		expected = 3;
> >  		name_len = attri_formatp->alfi_name_len;
> >  		value_len = attri_formatp->alfi_value_len;
> >  		break;
> >  	case XFS_ATTRI_OP_FLAGS_SET:
> >  	case XFS_ATTRI_OP_FLAGS_REPLACE:
> >  		/* Log item, attr name, 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;
> > -		}
> > +		expected = 2 + !!attri_formatp->alfi_value_len;
> >  		name_len = attri_formatp->alfi_name_len;
> >  		value_len = attri_formatp->alfi_value_len;
> >  		break;
> >  	case XFS_ATTRI_OP_FLAGS_REMOVE:
> >  		/* Log item, attr name */
> > -		if (item->ri_total != 2) {
> > -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> > -					     attri_formatp, len);
> > -			return -EFSCORRUPTED;
> > -		}
> > +		expected = 2;
> >  		name_len = attri_formatp->alfi_name_len;
> >  		break;
> >  	case XFS_ATTRI_OP_FLAGS_PPTR_REPLACE:
> > @@ -1071,11 +1060,7 @@ xlog_recover_attri_commit_pass2(
> >  		 * Log item, attr name, new attr name, attr value, new attr
> >  		 * value
> >  		 */
> > -		if (item->ri_total != 5) {
> > -			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> > -					     attri_formatp, len);
> > -			return -EFSCORRUPTED;
> > -		}
> > +		expected = 5;
> >  		name_len = attri_formatp->alfi_old_name_len;
> >  		new_name_len = attri_formatp->alfi_new_name_len;
> >  		new_value_len = value_len = attri_formatp->alfi_value_len;
> > @@ -1085,6 +1070,12 @@ xlog_recover_attri_commit_pass2(
> >  				     attri_formatp, len);
> >  		return -EFSCORRUPTED;
> >  	}
> > +
> > +	if (item->ri_total != expected) {
> > +		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> 
> The downside to this change is that XFS_CORRUPTION_ERROR prints the
> source file and line number, so now anyone looking through the logs
> cannot identify a specific line number.  However...
> 

The corrupted information will output the entire `xfs_attri_log_format`,
from which the operation type can also be inferred. However, the original
code outputs more intuitive information, and thus it might be fine to leave
it unchanged.

Thanks,
Long Li