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@xxxxxxxxxx>
> >
> > 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" ?
> 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);
>
>
