Re: [PATCH v4 01/27] xfs: Add new name to attri/d

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

On Fri, Oct 21, 2022 at 03:29:10PM -0700, allison.henderson@oracle.com wrote:
> From: Allison Henderson <allison.henderson@oracle.com>
> 
> This patch adds two new fields to the atti/d.  They are nname and
> nnamelen.  This will be used for parent pointer updates since a
> rename operation may cause the parent pointer to update both the
> name and value.  So we need to carry both the new name as well as
> the target name in the attri/d.
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c       |  12 +++-
>  fs/xfs/libxfs/xfs_attr.h       |   4 +-
>  fs/xfs/libxfs/xfs_da_btree.h   |   2 +
>  fs/xfs/libxfs/xfs_log_format.h |   6 +-
>  fs/xfs/xfs_attr_item.c         | 108 +++++++++++++++++++++++++++++----
>  fs/xfs/xfs_attr_item.h         |   1 +
>  6 files changed, 115 insertions(+), 18 deletions(-)

<snip all the way to the one thing I noticed>

> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index cf5ce607dc05..0c449fb606ed 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -731,10 +767,41 @@ xlog_recover_attri_commit_pass2(

Ahahaha this function.  Could you review this patch that strengthens the
length checking on the incoming recovery item buffers, please?

https://lore.kernel.org/linux-xfs/166664715731.2688790.9836328662603103847.stgit@magnolia/

>  	struct xfs_attri_log_nameval	*nv;
>  	const void			*attr_value = NULL;
>  	const void			*attr_name;
> -	int                             error;
> +	const void			*attr_nname = NULL;
> +	int				i = 0;
> +	int                             op, error = 0;
>  
> -	attri_formatp = item->ri_buf[0].i_addr;
> -	attr_name = item->ri_buf[1].i_addr;
> +	if (item->ri_total == 0) {

Do all the log intent item types need to check for a nonzero number of
recovery item buffers too?  I /think/ this is unnecessary because
xlog_recover_add_to_trans will abort recovery if ilf_size == 0, and
ri_total is assigned to ilf_size:

	if (item->ri_total == 0) {	/* first region to be added */
		if (in_f->ilf_size == 0 ||
		    in_f->ilf_size > XLOG_MAX_REGIONS_IN_ITEM) {
			xfs_warn(log->l_mp,
		"bad number of regions (%d) in inode log format",
				  in_f->ilf_size);
			ASSERT(0);
			kmem_free(ptr);
			return -EFSCORRUPTED;
		}

		item->ri_total = in_f->ilf_size;

Hm?

> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	attri_formatp = item->ri_buf[i].i_addr;
> +	i++;
> +
> +	op = attri_formatp->alfi_op_flags & XFS_ATTRI_OP_FLAGS_TYPE_MASK;
> +	switch (op) {
> +	case XFS_ATTRI_OP_FLAGS_SET:
> +	case XFS_ATTRI_OP_FLAGS_REPLACE:
> +		if (item->ri_total != 3)
> +			error = -EFSCORRUPTED;
> +		break;
> +	case XFS_ATTRI_OP_FLAGS_REMOVE:
> +		if (item->ri_total != 2)
> +			error = -EFSCORRUPTED;
> +		break;
> +	case XFS_ATTRI_OP_FLAGS_NVREPLACE:
> +		if (item->ri_total != 4)
> +			error = -EFSCORRUPTED;
> +		break;
> +	default:
> +		error = -EFSCORRUPTED;
> +	}
> +
> +	if (error) {
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);

XFS_ERROR_REPORT is a macro encodes the exact instruction pointer
location in the error report that it emits.  I know it'll make the code
more verbose, but the macros should be embedded in that switch statement
above.

> +		return error;
> +	}
>  
>  	/* Validate xfs_attri_log_format before the large memory allocation */
>  	if (!xfs_attri_validate(mp, attri_formatp)) {
> @@ -742,13 +809,27 @@ xlog_recover_attri_commit_pass2(
>  		return -EFSCORRUPTED;
>  	}
>  
> +	attr_name = item->ri_buf[i].i_addr;
> +	i++;
> +
>  	if (!xfs_attr_namecheck(attr_name, attri_formatp->alfi_name_len)) {
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
>  		return -EFSCORRUPTED;
>  	}
>  
> +	if (attri_formatp->alfi_nname_len) {

This needs to check that the length of the new name iovec buffer is what
we're expecting:

	if (item->ri_buf[i].i_len !=
			xlog_calc_iovec_len(attri_formatp->alfi_nname_len)) {
		/* complain... */
	}

--D

> +		attr_nname = item->ri_buf[i].i_addr;
> +		i++;
> +
> +		if (!xfs_attr_namecheck(attr_nname,
> +				attri_formatp->alfi_nname_len)) {
> +			XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> +			return -EFSCORRUPTED;
> +		}
> +	}
> +
>  	if (attri_formatp->alfi_value_len)
> -		attr_value = item->ri_buf[2].i_addr;
> +		attr_value = item->ri_buf[i].i_addr;
>  
>  	/*
>  	 * Memory alloc failure will cause replay to abort.  We attach the
> @@ -756,7 +837,8 @@ xlog_recover_attri_commit_pass2(
>  	 * reference.
>  	 */
>  	nv = xfs_attri_log_nameval_alloc(attr_name,
> -			attri_formatp->alfi_name_len, attr_value,
> +			attri_formatp->alfi_name_len, attr_nname,
> +			attri_formatp->alfi_nname_len, attr_value,
>  			attri_formatp->alfi_value_len);
>  
>  	attrip = xfs_attri_init(mp, nv);
> diff --git a/fs/xfs/xfs_attr_item.h b/fs/xfs/xfs_attr_item.h
> index 3280a7930287..24d4968dd6cc 100644
> --- a/fs/xfs/xfs_attr_item.h
> +++ b/fs/xfs/xfs_attr_item.h
> @@ -13,6 +13,7 @@ struct kmem_zone;
>  
>  struct xfs_attri_log_nameval {
>  	struct xfs_log_iovec	name;
> +	struct xfs_log_iovec	nname;
>  	struct xfs_log_iovec	value;
>  	refcount_t		refcount;
>  
> -- 
> 2.25.1
>