| Message ID | YpzbrQdA9voYKRCE@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] xfs: fix TOCTOU race involving the new logged xattrs control knob | expand |
On Sun, Jun 05, 2022 at 09:37:01AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > I found a race involving the larp control knob, aka the debugging knob > that lets developers enable logging of extended attribute updates: > > Thread 1 Thread 2 > > echo 0 > /sys/fs/xfs/debug/larp > setxattr(REPLACE) > xfs_has_larp (returns false) > xfs_attr_set > > echo 1 > /sys/fs/xfs/debug/larp > > xfs_attr_defer_replace > xfs_attr_init_replace_state > xfs_has_larp (returns true) > xfs_attr_init_remove_state > > <oops, wrong DAS state!> > > This isn't a particularly severe problem right now because xattr logging > is only enabled when CONFIG_XFS_DEBUG=y, and developers *should* know > what they're doing. > > However, the eventual intent is that callers should be able to ask for > the assistance of the log in persisting xattr updates. This capability > might not be required for /all/ callers, which means that dynamic > control must work correctly. Once an xattr update has decided whether > or not to use logged xattrs, it needs to stay in that mode until the end > of the operation regardless of what subsequent parallel operations might > do. > > Therefore, it is an error to continue sampling xfs_globals.larp once > xfs_attr_change has made a decision about larp, and it was not correct > for me to have told Allison that ->create_intent functions can sample > the global log incompat feature bitfield to decide to elide a log item. > > Instead, create a new op flag for the xfs_da_args structure, and convert > all other callers of xfs_has_larp and xfs_sb_version_haslogxattrs within > the attr update state machine to look for the operations flag. *nod* .... > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > index 4a28c2d77070..135d44133477 100644 > --- a/fs/xfs/xfs_attr_item.c > +++ b/fs/xfs/xfs_attr_item.c > @@ -413,18 +413,20 @@ xfs_attr_create_intent( > struct xfs_mount *mp = tp->t_mountp; > struct xfs_attri_log_item *attrip; > struct xfs_attr_intent *attr; > + struct xfs_da_args *args; > > ASSERT(count == 1); > > - if (!xfs_sb_version_haslogxattrs(&mp->m_sb)) > - return NULL; > - > /* > * Each attr item only performs one attribute operation at a time, so > * this is a list of one > */ > attr = list_first_entry_or_null(items, struct xfs_attr_intent, > xattri_list); > + args = attr->xattri_da_args; > + > + if (!(args->op_flags & XFS_DA_OP_LOGGED)) > + return NULL; Hmmmm. For the non-LARP case, do we need to be checking if (!attr) return NULL; now? > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > index 35e13e125ec6..149a8f537b06 100644 > --- a/fs/xfs/xfs_xattr.c > +++ b/fs/xfs/xfs_xattr.c > @@ -68,6 +68,18 @@ xfs_attr_rele_log_assist( > xlog_drop_incompat_feat(mp->m_log); > } > > +#ifdef DEBUG > +static inline bool > +xfs_attr_want_log_assist( > + struct xfs_mount *mp) > +{ > + /* Logged xattrs require a V5 super for log_incompat */ > + return xfs_has_crc(mp) && xfs_globals.larp; > +} > +#else > +# define xfs_attr_want_log_assist(mp) false > +#endif If you are moving this code, let's clean it up a touch so that it is the internal logic that is conditional, not the function itself. static inline bool xfs_attr_want_log_assist( struct xfs_mount *mp) { #ifdef DEBUG /* Logged xattrs require a V5 super for log_incompat */ return xfs_has_crc(mp) && xfs_globals.larp; #else return false; #endif } Cheers, Dave.
On Mon, Jun 06, 2022 at 08:47:43AM +1000, Dave Chinner wrote: > On Sun, Jun 05, 2022 at 09:37:01AM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > I found a race involving the larp control knob, aka the debugging knob > > that lets developers enable logging of extended attribute updates: > > > > Thread 1 Thread 2 > > > > echo 0 > /sys/fs/xfs/debug/larp > > setxattr(REPLACE) > > xfs_has_larp (returns false) > > xfs_attr_set > > > > echo 1 > /sys/fs/xfs/debug/larp > > > > xfs_attr_defer_replace > > xfs_attr_init_replace_state > > xfs_has_larp (returns true) > > xfs_attr_init_remove_state > > > > <oops, wrong DAS state!> > > > > This isn't a particularly severe problem right now because xattr logging > > is only enabled when CONFIG_XFS_DEBUG=y, and developers *should* know > > what they're doing. > > > > However, the eventual intent is that callers should be able to ask for > > the assistance of the log in persisting xattr updates. This capability > > might not be required for /all/ callers, which means that dynamic > > control must work correctly. Once an xattr update has decided whether > > or not to use logged xattrs, it needs to stay in that mode until the end > > of the operation regardless of what subsequent parallel operations might > > do. > > > > Therefore, it is an error to continue sampling xfs_globals.larp once > > xfs_attr_change has made a decision about larp, and it was not correct > > for me to have told Allison that ->create_intent functions can sample > > the global log incompat feature bitfield to decide to elide a log item. > > > > Instead, create a new op flag for the xfs_da_args structure, and convert > > all other callers of xfs_has_larp and xfs_sb_version_haslogxattrs within > > the attr update state machine to look for the operations flag. > > *nod* > > .... > > > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c > > index 4a28c2d77070..135d44133477 100644 > > --- a/fs/xfs/xfs_attr_item.c > > +++ b/fs/xfs/xfs_attr_item.c > > @@ -413,18 +413,20 @@ xfs_attr_create_intent( > > struct xfs_mount *mp = tp->t_mountp; > > struct xfs_attri_log_item *attrip; > > struct xfs_attr_intent *attr; > > + struct xfs_da_args *args; > > > > ASSERT(count == 1); > > > > - if (!xfs_sb_version_haslogxattrs(&mp->m_sb)) > > - return NULL; > > - > > /* > > * Each attr item only performs one attribute operation at a time, so > > * this is a list of one > > */ > > attr = list_first_entry_or_null(items, struct xfs_attr_intent, > > xattri_list); > > + args = attr->xattri_da_args; > > + > > + if (!(args->op_flags & XFS_DA_OP_LOGGED)) > > + return NULL; > > Hmmmm. For the non-LARP case, do we need to be checking > > if (!attr) > return NULL; > > now? I don't think that's necessary, since the struct xfs_attr_intent is always allocated and used to track the incore state of the operation, even when we aren't going to use the log items. > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c > > index 35e13e125ec6..149a8f537b06 100644 > > --- a/fs/xfs/xfs_xattr.c > > +++ b/fs/xfs/xfs_xattr.c > > @@ -68,6 +68,18 @@ xfs_attr_rele_log_assist( > > xlog_drop_incompat_feat(mp->m_log); > > } > > > > +#ifdef DEBUG > > +static inline bool > > +xfs_attr_want_log_assist( > > + struct xfs_mount *mp) > > +{ > > + /* Logged xattrs require a V5 super for log_incompat */ > > + return xfs_has_crc(mp) && xfs_globals.larp; > > +} > > +#else > > +# define xfs_attr_want_log_assist(mp) false > > +#endif > > If you are moving this code, let's clean it up a touch so that it > is the internal logic that is conditional, not the function itself. > > static inline bool > xfs_attr_want_log_assist( > struct xfs_mount *mp) > { > #ifdef DEBUG > /* Logged xattrs require a V5 super for log_incompat */ > return xfs_has_crc(mp) && xfs_globals.larp; > #else > return false; > #endif > } I don't mind turning this into a straight function move. I'd figured that Linus' style preference is usually against putting conditional compilation inside functions, but for a static inline I really don't care either way. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Sun, Jun 05, 2022 at 05:08:46PM -0700, Darrick J. Wong wrote: > On Mon, Jun 06, 2022 at 08:47:43AM +1000, Dave Chinner wrote: > > On Sun, Jun 05, 2022 at 09:37:01AM -0700, Darrick J. Wong wrote: > > > --- a/fs/xfs/xfs_xattr.c > > > +++ b/fs/xfs/xfs_xattr.c > > > @@ -68,6 +68,18 @@ xfs_attr_rele_log_assist( > > > xlog_drop_incompat_feat(mp->m_log); > > > } > > > > > > +#ifdef DEBUG > > > +static inline bool > > > +xfs_attr_want_log_assist( > > > + struct xfs_mount *mp) > > > +{ > > > + /* Logged xattrs require a V5 super for log_incompat */ > > > + return xfs_has_crc(mp) && xfs_globals.larp; > > > +} > > > +#else > > > +# define xfs_attr_want_log_assist(mp) false > > > +#endif > > > > If you are moving this code, let's clean it up a touch so that it > > is the internal logic that is conditional, not the function itself. > > > > static inline bool > > xfs_attr_want_log_assist( > > struct xfs_mount *mp) > > { > > #ifdef DEBUG > > /* Logged xattrs require a V5 super for log_incompat */ > > return xfs_has_crc(mp) && xfs_globals.larp; > > #else > > return false; > > #endif > > } > > I don't mind turning this into a straight function move. I'd figured > that Linus' style preference is usually against putting conditional > compilation inside functions, but for a static inline I really don't > care either way. Well, for conditional helper functions, having the static inline for type checking in all builds is better than having a macro that makes it go away silently when those build options are not turned on. Better would probably be: if (!IS_ENABLED(CONFIG_XFS_DEBUG)) return false; /* Logged xattrs require a V5 super for log_incompat */ return xfs_has_crc(mp) && xfs_globals.larp; And all the ifdef mess goes away at that point. Cheers, Dave.
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index e329da3e7afa..b4a2fc77017e 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -28,16 +28,6 @@ struct xfs_attr_list_context; */ #define ATTR_MAX_VALUELEN (64*1024) /* max length of a value */ -static inline bool xfs_has_larp(struct xfs_mount *mp) -{ -#ifdef DEBUG - /* Logged xattrs require a V5 super for log_incompat */ - return xfs_has_crc(mp) && xfs_globals.larp; -#else - return false; -#endif -} - /* * Kernel-internal version of the attrlist cursor. */ @@ -624,7 +614,7 @@ static inline enum xfs_delattr_state xfs_attr_init_replace_state(struct xfs_da_args *args) { args->op_flags |= XFS_DA_OP_ADDNAME | XFS_DA_OP_REPLACE; - if (xfs_has_larp(args->dp->i_mount)) + if (args->op_flags & XFS_DA_OP_LOGGED) return xfs_attr_init_remove_state(args); return xfs_attr_init_add_state(args); } diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 15a990409463..37e7c33f6283 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -1530,7 +1530,7 @@ xfs_attr3_leaf_add_work( if (tmp) entry->flags |= XFS_ATTR_LOCAL; if (args->op_flags & XFS_DA_OP_REPLACE) { - if (!xfs_has_larp(mp)) + if (!(args->op_flags & XFS_DA_OP_LOGGED)) entry->flags |= XFS_ATTR_INCOMPLETE; if ((args->blkno2 == args->blkno) && (args->index2 <= args->index)) { diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h index d33b7686a0b3..033c35e71c85 100644 --- a/fs/xfs/libxfs/xfs_da_btree.h +++ b/fs/xfs/libxfs/xfs_da_btree.h @@ -92,6 +92,7 @@ typedef struct xfs_da_args { #define XFS_DA_OP_NOTIME (1u << 5) /* don't update inode timestamps */ #define XFS_DA_OP_REMOVE (1u << 6) /* this is a remove operation */ #define XFS_DA_OP_RECOVERY (1u << 7) /* Log recovery operation */ +#define XFS_DA_OP_LOGGED (1u << 8) /* Use intent items to track op */ #define XFS_DA_OP_FLAGS \ { XFS_DA_OP_JUSTCHECK, "JUSTCHECK" }, \ @@ -101,7 +102,8 @@ typedef struct xfs_da_args { { XFS_DA_OP_CILOOKUP, "CILOOKUP" }, \ { XFS_DA_OP_NOTIME, "NOTIME" }, \ { XFS_DA_OP_REMOVE, "REMOVE" }, \ - { XFS_DA_OP_RECOVERY, "RECOVERY" } + { XFS_DA_OP_RECOVERY, "RECOVERY" }, \ + { XFS_DA_OP_LOGGED, "LOGGED" } /* * Storage for holding state during Btree searches and split/join ops. diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c index 4a28c2d77070..135d44133477 100644 --- a/fs/xfs/xfs_attr_item.c +++ b/fs/xfs/xfs_attr_item.c @@ -413,18 +413,20 @@ xfs_attr_create_intent( struct xfs_mount *mp = tp->t_mountp; struct xfs_attri_log_item *attrip; struct xfs_attr_intent *attr; + struct xfs_da_args *args; ASSERT(count == 1); - if (!xfs_sb_version_haslogxattrs(&mp->m_sb)) - return NULL; - /* * Each attr item only performs one attribute operation at a time, so * this is a list of one */ attr = list_first_entry_or_null(items, struct xfs_attr_intent, xattri_list); + args = attr->xattri_da_args; + + if (!(args->op_flags & XFS_DA_OP_LOGGED)) + return NULL; /* * Create a buffer to store the attribute name and value. This buffer @@ -432,8 +434,6 @@ xfs_attr_create_intent( * and the lower level xattr log items. */ if (!attr->xattri_nameval) { - struct xfs_da_args *args = attr->xattri_da_args; - /* * Transfer our reference to the name/value buffer to the * deferred work state structure. @@ -617,7 +617,10 @@ xfs_attri_item_recover( args->namelen = nv->name.i_len; args->hashval = xfs_da_hashname(args->name, args->namelen); args->attr_filter = attrp->alfi_attr_filter & XFS_ATTRI_FILTER_MASK; - args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT; + args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT | + XFS_DA_OP_LOGGED; + + ASSERT(xfs_sb_version_haslogxattrs(&mp->m_sb)); switch (attr->xattri_op_flags) { case XFS_ATTRI_OP_FLAGS_SET: diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c index 35e13e125ec6..149a8f537b06 100644 --- a/fs/xfs/xfs_xattr.c +++ b/fs/xfs/xfs_xattr.c @@ -68,6 +68,18 @@ xfs_attr_rele_log_assist( xlog_drop_incompat_feat(mp->m_log); } +#ifdef DEBUG +static inline bool +xfs_attr_want_log_assist( + struct xfs_mount *mp) +{ + /* Logged xattrs require a V5 super for log_incompat */ + return xfs_has_crc(mp) && xfs_globals.larp; +} +#else +# define xfs_attr_want_log_assist(mp) false +#endif + /* * Set or remove an xattr, having grabbed the appropriate logging resources * prior to calling libxfs. @@ -80,11 +92,14 @@ xfs_attr_change( bool use_logging = false; int error; - if (xfs_has_larp(mp)) { + ASSERT(!(args->op_flags & XFS_DA_OP_LOGGED)); + + if (xfs_attr_want_log_assist(mp)) { error = xfs_attr_grab_log_assist(mp); if (error) return error; + args->op_flags |= XFS_DA_OP_LOGGED; use_logging = true; }