Re: [PATCH 4/4] xfs: reject unknown xattri log item filter flags during recovery

[Alli <allison.henderson@oracle.com>]

On Wed, 2022-05-18 at 11:54 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure we screen the "attr flags" field of recovered xattr intent
> log
> items to reject flag bits that we don't know about.  This is really
> the
> attr *filter* field from xfs_da_args, so rename the field and create
> a mask to make checking for invalid bits easier.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks ok now
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_log_format.h |   10 +++++++++-
>  fs/xfs/xfs_attr_item.c         |   10 +++++++---
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h
> b/fs/xfs/libxfs/xfs_log_format.h
> index f7edd1ecf6d9..a9d08f3d4682 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -911,6 +911,14 @@ struct xfs_icreate_log {
>  #define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
>  #define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
>  
> +/*
> + * alfi_attr_filter captures the state of xfs_da_args.attr_filter,
> so it should
> + * never have any other bits set.
> + */
> +#define XFS_ATTRI_FILTER_MASK		(XFS_ATTR_ROOT | \
> +					 XFS_ATTR_SECURE | \
> +					 XFS_ATTR_INCOMPLETE)
> +
>  /*
>   * This is the structure used to lay out an attr log item in the
>   * log.
> @@ -924,7 +932,7 @@ struct xfs_attri_log_format {
>  	uint32_t	alfi_op_flags;	/* marks the op as a set or remove */
>  	uint32_t	alfi_name_len;	/* attr name length */
>  	uint32_t	alfi_value_len;	/* attr value length */
> -	uint32_t	alfi_attr_flags;/* attr flags */
> +	uint32_t	alfi_attr_filter;/* attr filter flags */
>  };
>  
>  struct xfs_attrd_log_format {
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index ae227a56bbed..fd0a74f3ef45 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -353,7 +353,8 @@ xfs_attr_log_item(
>  	attrp->alfi_op_flags = attr->xattri_op_flags;
>  	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
>  	attrp->alfi_name_len = attr->xattri_da_args->namelen;
> -	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
> +	ASSERT(!(attr->xattri_da_args->attr_filter &
> ~XFS_ATTRI_FILTER_MASK));
> +	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter;
>  
>  	memcpy(attrip->attri_name, attr->xattri_da_args->name,
>  	       attr->xattri_da_args->namelen);
> @@ -500,6 +501,9 @@ xfs_attri_validate(
>  	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
>  		return false;
>  
> +	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
> +		return false;
> +
>  	/* alfi_op_flags should be either a set or remove */
>  	switch (op) {
>  	case XFS_ATTR_OP_FLAGS_SET:
> @@ -569,7 +573,7 @@ xfs_attri_item_recover(
>  	args->name = attrip->attri_name;
>  	args->namelen = attrp->alfi_name_len;
>  	args->hashval = xfs_da_hashname(args->name, args->namelen);
> -	args->attr_filter = attrp->alfi_attr_flags;
> +	args->attr_filter = attrp->alfi_attr_filter &
> XFS_ATTRI_FILTER_MASK;
>  	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
>  
>  	switch (attr->xattri_op_flags) {
> @@ -658,7 +662,7 @@ xfs_attri_item_relog(
>  	new_attrp->alfi_op_flags = old_attrp->alfi_op_flags;
>  	new_attrp->alfi_value_len = old_attrp->alfi_value_len;
>  	new_attrp->alfi_name_len = old_attrp->alfi_name_len;
> -	new_attrp->alfi_attr_flags = old_attrp->alfi_attr_flags;
> +	new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
>  
>  	memcpy(new_attrip->attri_name, old_attrip->attri_name,
>  		new_attrip->attri_name_len);
>