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

["Darrick J. Wong" <djwong@kernel.org>]

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
> 
>