Re: [PATCH 3/4] xfs: rename xfs_da_args.attr_flags

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

On Wed, Apr 10, 2024 at 01:55:28PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 09, 2024 at 10:01:55PM -0700, Christoph Hellwig wrote:
> > On Tue, Apr 09, 2024 at 05:50:07PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > This field only ever contains XATTR_{CREATE,REPLACE}, so let's change
> > > the name of the field to make the field and its values consistent.
> > 
> > So, these flags only get passed to xfs_attr_set through xfs_attr_change
> > and xfs_attr_setname, which means we should probably just pass them
> > directly as in my patch (against your whole stack) below.
> 
> Want me to reflow this through the tree, or just tack it on the end
> after (perhaps?) "xfs: fix corruptions in the directory tree" ?

Ugh, no, that got messy so I just tacked it on the end. :)

Also I changed the uint8_t parameter to int because the XATTR_* flags
mostly come from the VFS and that's what it passes us in
xattr_handler::set().

--D

> > Also I suspect we should do an audit of all the internal callers
> > if they should ever be replace an existing attr, as I guess most
> > don't.  (and xfs_attr_change really should be folded into xfs_attr_set,
> > the split is confusing as hell).
> 
> I imagine a lot of the security stuff with magic xattrs probably only
> ever creates xattrs, but I would bet that some of these subsystems
> actually *want* the upsert behavior -- "the frob for this file should be
> $foo, make it so".
> 
> --D
> 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index b98d2a908452a0..38d1f4d10baa3b 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -1034,7 +1034,8 @@ xfs_attr_ensure_iext(
> >   */
> >  int
> >  xfs_attr_set(
> > -	struct xfs_da_args	*args)
> > +	struct xfs_da_args	*args,
> > +	uint8_t			xattr_flags)
> >  {
> >  	struct xfs_inode	*dp = args->dp;
> >  	struct xfs_mount	*mp = dp->i_mount;
> > @@ -1109,7 +1110,7 @@ xfs_attr_set(
> >  		}
> >  
> >  		/* Pure create fails if the attr already exists */
> > -		if (args->xattr_flags & XATTR_CREATE)
> > +		if (xattr_flags & XATTR_CREATE)
> >  			goto out_trans_cancel;
> >  		xfs_attr_defer_add(args, XFS_ATTR_DEFER_REPLACE);
> >  		break;
> > @@ -1119,7 +1120,7 @@ xfs_attr_set(
> >  			goto out_trans_cancel;
> >  
> >  		/* Pure replace fails if no existing attr to replace. */
> > -		if (args->xattr_flags & XATTR_REPLACE)
> > +		if (xattr_flags & XATTR_REPLACE)
> >  			goto out_trans_cancel;
> >  		xfs_attr_defer_add(args, XFS_ATTR_DEFER_SET);
> >  		break;
> > @@ -1155,7 +1156,7 @@ xfs_attr_set(
> >   * Ensure that the xattr structure maps @args->name to @args->value.
> >   *
> >   * The caller must have initialized @args, attached dquots, and must not hold
> > - * any ILOCKs.  Only XATTR_CREATE may be specified in @args->xattr_flags.
> > + * any ILOCKs.  Only XATTR_CREATE may be specified in @xattr_flags.
> >   * Reserved data blocks may be used if @rsvd is set.
> >   *
> >   * Returns -EEXIST if XATTR_CREATE was specified and the name already exists.
> > @@ -1163,6 +1164,7 @@ xfs_attr_set(
> >  int
> >  xfs_attr_setname(
> >  	struct xfs_da_args	*args,
> > +	uint8_t			xattr_flags,
> >  	bool			rsvd)
> >  {
> >  	struct xfs_inode	*dp = args->dp;
> > @@ -1172,7 +1174,7 @@ xfs_attr_setname(
> >  	int			rmt_extents = 0;
> >  	int			error, local;
> >  
> > -	ASSERT(!(args->xattr_flags & XATTR_REPLACE));
> > +	ASSERT(!(xattr_flags & ~XATTR_CREATE));
> >  	ASSERT(!args->trans);
> >  
> >  	args->total = xfs_attr_calc_size(args, &local);
> > @@ -1198,7 +1200,7 @@ xfs_attr_setname(
> >  	switch (error) {
> >  	case -EEXIST:
> >  		/* Pure create fails if the attr already exists */
> > -		if (args->xattr_flags & XATTR_CREATE)
> > +		if (xattr_flags & XATTR_CREATE)
> >  			goto out_trans_cancel;
> >  		if (args->attr_filter & XFS_ATTR_PARENT)
> >  			xfs_attr_defer_parent(args, XFS_ATTR_DEFER_REPLACE);
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 2a0ef4f633e2d1..b90e04c3e64f60 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -550,7 +550,7 @@ int xfs_inode_hasattr(struct xfs_inode *ip);
> >  bool xfs_attr_is_leaf(struct xfs_inode *ip);
> >  int xfs_attr_get_ilocked(struct xfs_da_args *args);
> >  int xfs_attr_get(struct xfs_da_args *args);
> > -int xfs_attr_set(struct xfs_da_args *args);
> > +int xfs_attr_set(struct xfs_da_args *args, uint8_t xattr_flags);
> >  int xfs_attr_set_iter(struct xfs_attr_intent *attr);
> >  int xfs_attr_remove_iter(struct xfs_attr_intent *attr);
> >  bool xfs_attr_check_namespace(unsigned int attr_flags);
> > @@ -560,7 +560,7 @@ int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
> >  void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
> >  			 unsigned int *total);
> >  
> > -int xfs_attr_setname(struct xfs_da_args *args, bool rsvd);
> > +int xfs_attr_setname(struct xfs_da_args *args, uint8_t xattr_flags, bool rsvd);
> >  int xfs_attr_removename(struct xfs_da_args *args, bool rsvd);
> >  
> >  /*
> > diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
> > index 8d7a38fe2a5c07..354d5d65043e43 100644
> > --- a/fs/xfs/libxfs/xfs_da_btree.h
> > +++ b/fs/xfs/libxfs/xfs_da_btree.h
> > @@ -69,7 +69,6 @@ typedef struct xfs_da_args {
> >  	uint8_t		filetype;	/* filetype of inode for directories */
> >  	uint8_t		op_flags;	/* operation flags */
> >  	uint8_t		attr_filter;	/* XFS_ATTR_{ROOT,SECURE,INCOMPLETE} */
> > -	uint8_t		xattr_flags;	/* XATTR_{CREATE,REPLACE} */
> >  	short		namelen;	/* length of string (maybe no NULL) */
> >  	short		new_namelen;	/* length of new attr name */
> >  	xfs_dahash_t	hashval;	/* hash value of name */
> > diff --git a/fs/xfs/libxfs/xfs_parent.c b/fs/xfs/libxfs/xfs_parent.c
> > index 2b6ed8c1ee1522..c5422f714fcc72 100644
> > --- a/fs/xfs/libxfs/xfs_parent.c
> > +++ b/fs/xfs/libxfs/xfs_parent.c
> > @@ -355,7 +355,7 @@ xfs_parent_set(
> >  
> >  	memset(scratch, 0, sizeof(struct xfs_da_args));
> >  	xfs_parent_da_args_init(scratch, NULL, pptr, ip, owner, parent_name);
> > -	return xfs_attr_setname(scratch, true);
> > +	return xfs_attr_setname(scratch, 0, true);
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/scrub/attr_repair.c b/fs/xfs/scrub/attr_repair.c
> > index e06d00ea828b3e..8863eef5a0b87b 100644
> > --- a/fs/xfs/scrub/attr_repair.c
> > +++ b/fs/xfs/scrub/attr_repair.c
> > @@ -615,7 +615,6 @@ xrep_xattr_insert_rec(
> >  	struct xfs_da_args		args = {
> >  		.dp			= rx->sc->tempip,
> >  		.attr_filter		= key->flags,
> > -		.xattr_flags		= XATTR_CREATE,
> >  		.namelen		= key->namelen,
> >  		.valuelen		= key->valuelen,
> >  		.owner			= rx->sc->ip->i_ino,
> > @@ -675,7 +674,7 @@ xrep_xattr_insert_rec(
> >  	 * use reserved blocks because we can abort the repair with ENOSPC.
> >  	 */
> >  	xfs_attr_sethash(&args);
> > -	error = xfs_attr_setname(&args, false);
> > +	error = xfs_attr_setname(&args, XATTR_CREATE, false);
> >  	if (error == -EEXIST)
> >  		error = 0;
> >  
> > diff --git a/fs/xfs/scrub/parent_repair.c b/fs/xfs/scrub/parent_repair.c
> > index cf79cbcda3ecb4..1bc05efa344036 100644
> > --- a/fs/xfs/scrub/parent_repair.c
> > +++ b/fs/xfs/scrub/parent_repair.c
> > @@ -1031,7 +1031,7 @@ xrep_parent_insert_xattr(
> >  			rp->xattr_name, key->namelen, key->valuelen);
> >  
> >  	xfs_attr_sethash(&args);
> > -	return xfs_attr_setname(&args, false);
> > +	return xfs_attr_setname(&args, 0, false);
> >  }
> >  
> >  /*
> > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> > index 4bf69c9c088e28..1aaf3dc64bcbc1 100644
> > --- a/fs/xfs/xfs_acl.c
> > +++ b/fs/xfs/xfs_acl.c
> > @@ -203,7 +203,7 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >  		xfs_acl_to_disk(args.value, acl);
> >  	}
> >  
> > -	error = xfs_attr_change(&args);
> > +	error = xfs_attr_change(&args, 0);
> >  	kvfree(args.value);
> >  
> >  	/*
> > diff --git a/fs/xfs/xfs_handle.c b/fs/xfs/xfs_handle.c
> > index 833b0d7d8bea1c..e3f54817b91557 100644
> > --- a/fs/xfs/xfs_handle.c
> > +++ b/fs/xfs/xfs_handle.c
> > @@ -492,7 +492,6 @@ xfs_attrmulti_attr_get(
> >  	struct xfs_da_args	args = {
> >  		.dp		= XFS_I(inode),
> >  		.attr_filter	= xfs_attr_filter(flags),
> > -		.xattr_flags	= xfs_xattr_flags(flags),
> >  		.name		= name,
> >  		.namelen	= strlen(name),
> >  		.valuelen	= *len,
> > @@ -526,7 +525,6 @@ xfs_attrmulti_attr_set(
> >  	struct xfs_da_args	args = {
> >  		.dp		= XFS_I(inode),
> >  		.attr_filter	= xfs_attr_filter(flags),
> > -		.xattr_flags	= xfs_xattr_flags(flags),
> >  		.name		= name,
> >  		.namelen	= strlen(name),
> >  	};
> > @@ -544,7 +542,7 @@ xfs_attrmulti_attr_set(
> >  		args.valuelen = len;
> >  	}
> >  
> > -	error = xfs_attr_change(&args);
> > +	error = xfs_attr_change(&args, xfs_xattr_flags(flags));
> >  	if (!error && (flags & XFS_IOC_ATTR_ROOT))
> >  		xfs_forget_acl(inode, name);
> >  	kfree(args.value);
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index c4f9c7eec83590..d374be9f8a6e3e 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -64,7 +64,7 @@ xfs_initxattrs(
> >  			.value		= xattr->value,
> >  			.valuelen	= xattr->value_len,
> >  		};
> > -		error = xfs_attr_change(&args);
> > +		error = xfs_attr_change(&args, 0);
> >  		if (error < 0)
> >  			break;
> >  	}
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index dc074240ad239f..1292d69087dc0c 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -2131,7 +2131,6 @@ DECLARE_EVENT_CLASS(xfs_attr_class,
> >  		__field(int, valuelen)
> >  		__field(xfs_dahash_t, hashval)
> >  		__field(unsigned int, attr_filter)
> > -		__field(unsigned int, xattr_flags)
> >  		__field(uint32_t, op_flags)
> >  	),
> >  	TP_fast_assign(
> > @@ -2143,11 +2142,10 @@ DECLARE_EVENT_CLASS(xfs_attr_class,
> >  		__entry->valuelen = args->valuelen;
> >  		__entry->hashval = args->hashval;
> >  		__entry->attr_filter = args->attr_filter;
> > -		__entry->xattr_flags = args->xattr_flags;
> >  		__entry->op_flags = args->op_flags;
> >  	),
> >  	TP_printk("dev %d:%d ino 0x%llx name %.*s namelen %d valuelen %d "
> > -		  "hashval 0x%x filter %s flags %s op_flags %s",
> > +		  "hashval 0x%x filter %s op_flags %s",
> >  		  MAJOR(__entry->dev), MINOR(__entry->dev),
> >  		  __entry->ino,
> >  		  __entry->namelen,
> > @@ -2157,9 +2155,6 @@ DECLARE_EVENT_CLASS(xfs_attr_class,
> >  		  __entry->hashval,
> >  		  __print_flags(__entry->attr_filter, "|",
> >  				XFS_ATTR_FILTER_FLAGS),
> > -		   __print_flags(__entry->xattr_flags, "|",
> > -				{ XATTR_CREATE,		"CREATE" },
> > -				{ XATTR_REPLACE,	"REPLACE" }),
> >  		  __print_flags(__entry->op_flags, "|", XFS_DA_OP_FLAGS))
> >  )
> >  
> > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> > index 1d57e204c850ff..69fa7b89c68972 100644
> > --- a/fs/xfs/xfs_xattr.c
> > +++ b/fs/xfs/xfs_xattr.c
> > @@ -80,7 +80,8 @@ xfs_attr_want_log_assist(
> >   */
> >  int
> >  xfs_attr_change(
> > -	struct xfs_da_args	*args)
> > +	struct xfs_da_args	*args,
> > +	uint8_t			xattr_flags)
> >  {
> >  	struct xfs_mount	*mp = args->dp->i_mount;
> >  	int			error;
> > @@ -95,7 +96,7 @@ xfs_attr_change(
> >  		args->op_flags |= XFS_DA_OP_LOGGED;
> >  	}
> >  
> > -	return xfs_attr_set(args);
> > +	return xfs_attr_set(args, xattr_flags);
> >  }
> >  
> >  
> > @@ -131,7 +132,6 @@ xfs_xattr_set(const struct xattr_handler *handler,
> >  	struct xfs_da_args	args = {
> >  		.dp		= XFS_I(inode),
> >  		.attr_filter	= handler->flags,
> > -		.xattr_flags	= flags,
> >  		.name		= name,
> >  		.namelen	= strlen(name),
> >  		.value		= (void *)value,
> > @@ -139,7 +139,7 @@ xfs_xattr_set(const struct xattr_handler *handler,
> >  	};
> >  	int			error;
> >  
> > -	error = xfs_attr_change(&args);
> > +	error = xfs_attr_change(&args, flags);
> >  	if (!error && (handler->flags & XFS_ATTR_ROOT))
> >  		xfs_forget_acl(inode, name);
> >  	return error;
> > diff --git a/fs/xfs/xfs_xattr.h b/fs/xfs/xfs_xattr.h
> > index f097002d06571f..79c0040cc904b4 100644
> > --- a/fs/xfs/xfs_xattr.h
> > +++ b/fs/xfs/xfs_xattr.h
> > @@ -6,7 +6,7 @@
> >  #ifndef __XFS_XATTR_H__
> >  #define __XFS_XATTR_H__
> >  
> > -int xfs_attr_change(struct xfs_da_args *args);
> > +int xfs_attr_change(struct xfs_da_args *args, uint8_t xattr_flags);
> >  int xfs_attr_grab_log_assist(struct xfs_mount *mp);
> >  void xfs_attr_rele_log_assist(struct xfs_mount *mp);
> >  
> > 
>