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

[PATCH v2 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.

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 (3):
  xfs: fix possible null pointer dereference in xfs_attri_recover_work
  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 | 53 ++++--------------------------------------
 1 file changed, 4 insertions(+), 49 deletions(-)

-- 
2.39.2

[PATCH v2 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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 354472bf45f1..8ebdd0926b89 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -633,7 +633,7 @@ xfs_attri_recover_work(
 {
 	struct xfs_attr_intent		*attr;
 	struct xfs_da_args		*args;
-	struct xfs_inode		*ip;
+	struct xfs_inode		*ip = NULL;
 	int				local;
 	int				error;
 
@@ -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 v2 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.

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
 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/  # 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")
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 8ebdd0926b89..19eaf6b8cd43 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

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

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 19eaf6b8cd43..b462e15d2329 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -1133,52 +1133,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 v2 1/3] xfs: fix possible null pointer dereference in xfs_attri_recover_work

[Darrick J. Wong]

On Thu, Mar 19, 2026 at 09:06:16AM +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.
> 
> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 354472bf45f1..8ebdd0926b89 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -633,7 +633,7 @@ xfs_attri_recover_work(
>  {
>  	struct xfs_attr_intent		*attr;
>  	struct xfs_da_args		*args;
> -	struct xfs_inode		*ip;
> +	struct xfs_inode		*ip = NULL;
>  	int				local;
>  	int				error;
>  
> @@ -653,7 +653,8 @@ xfs_attri_recover_work(
>  		break;
>  	}
>  	if (error) {
> -		xfs_irele(ip);
> +		if (ip)
> +			xfs_irele(ip);

Hrmm.  On second thought, there's a much more severe UAF bug here:

int
xlog_recover_iget(
	struct xfs_mount	*mp,
	xfs_ino_t		ino,
	struct xfs_inode	**ipp)
{
	int			error;

	error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
	if (error)
		return error;

	error = xfs_qm_dqattach(*ipp);
	if (error) {
		xfs_irele(*ipp);
		return error;

^^^^^ here we return a nonzero error, having previously set @ipp.
The xfs_irele in xfs_attri_recover_work is, in this case, the wrong
thing to do.

	}

	if (VFS_I(*ipp)->i_nlink == 0)
		xfs_iflags_set(*ipp, XFS_IRECOVERY);

	return 0;
}

With that fixed, the xfs_irele call in xfs_attri_recover_work becomes
incorrect because the xlog*iget functions never return nonzero *and* set
*ipp.  If you found this via static checker, I wonder if that's what is
tripping it up?

--D

>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
>  				sizeof(*attrp));
>  		return ERR_PTR(-EFSCORRUPTED);
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 2/3] xfs: fix ri_total validation in xlog_recover_attri_commit_pass2

[Darrick J. Wong]

On Thu, Mar 19, 2026 at 09:06:17AM +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.
> 
> 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
>  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/  # 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")
> 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 8ebdd0926b89..19eaf6b8cd43 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) {

The comment should be updated:

		/* Log item, attr name, optional attr value */

With that fixed,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
>  					     attri_formatp, len);
>  			return -EFSCORRUPTED;
> -- 
> 2.39.2
> 
> 

Re: [PATCH v2 3/3] xfs: remove redundant validation in xlog_recover_attri_commit_pass2

[Darrick J. Wong]

On Thu, Mar 19, 2026 at 09:06:18AM +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.
> 
> Signed-off-by: Long Li <leo.lilong@huawei.com>

Yeah, that was redundant.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  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 19eaf6b8cd43..b462e15d2329 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -1133,52 +1133,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 v2 1/3] xfs: fix possible null pointer dereference in xfs_attri_recover_work

[Long Li]

On Thu, Mar 19, 2026 at 09:52:00AM -0700, Darrick J. Wong wrote:
> On Thu, Mar 19, 2026 at 09:06:16AM +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.
> > 
> > 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 | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 354472bf45f1..8ebdd0926b89 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -633,7 +633,7 @@ xfs_attri_recover_work(
> >  {
> >  	struct xfs_attr_intent		*attr;
> >  	struct xfs_da_args		*args;
> > -	struct xfs_inode		*ip;
> > +	struct xfs_inode		*ip = NULL;
> >  	int				local;
> >  	int				error;
> >  
> > @@ -653,7 +653,8 @@ xfs_attri_recover_work(
> >  		break;
> >  	}
> >  	if (error) {
> > -		xfs_irele(ip);
> > +		if (ip)
> > +			xfs_irele(ip);
> 
> Hrmm.  On second thought, there's a much more severe UAF bug here:
> 
> int
> xlog_recover_iget(
> 	struct xfs_mount	*mp,
> 	xfs_ino_t		ino,
> 	struct xfs_inode	**ipp)
> {
> 	int			error;
> 
> 	error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
> 	if (error)
> 		return error;
> 
> 	error = xfs_qm_dqattach(*ipp);
> 	if (error) {
> 		xfs_irele(*ipp);
> 		return error;
> 
> ^^^^^ here we return a nonzero error, having previously set @ipp.
> The xfs_irele in xfs_attri_recover_work is, in this case, the wrong
> thing to do.
> 
> 	}
> 
> 	if (VFS_I(*ipp)->i_nlink == 0)
> 		xfs_iflags_set(*ipp, XFS_IRECOVERY);
> 
> 	return 0;
> }
> 
> With that fixed, the xfs_irele call in xfs_attri_recover_work becomes
> incorrect because the xlog*iget functions never return nonzero *and* set
> *ipp.  If you found this via static checker, I wonder if that's what is
> tripping it up?
> 

Yes, we found this through static analysis. Obviously, I didn't look
at this code carefully enough. Thank you for pointing it out and fixing
it.

Thanks,
Long Li