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

[Long Li <leo.lilong@huawei.com>]

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