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