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

[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.
-- 
Dave Chinner
david@fromorbit.com