Re: [PATCH 1/2] xfs: fix TOCTOU race involving the new logged xattrs control knob

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

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