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

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

[Darrick J. Wong]

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.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.h      |   12 +-----------
 fs/xfs/libxfs/xfs_attr_leaf.c |    2 +-
 fs/xfs/libxfs/xfs_da_btree.h  |    4 +++-
 fs/xfs/xfs_attr_item.c        |   15 +++++++++------
 fs/xfs/xfs_xattr.c            |   17 ++++++++++++++++-
 5 files changed, 30 insertions(+), 20 deletions(-)

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;
 	}
 

[PATCH 2/2] xfs: fix variable state usage control knob

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The variable @args is fed to a tracepoint, and that's the only place
it's used.  This is fine for the kernel, but for userspace, tracepoints
are #define'd out of existence, which results in this warning on gcc
11.2:

xfs_attr.c: In function ‘xfs_attr_node_try_addname’:
xfs_attr.c:1440:42: warning: unused variable ‘args’ [-Wunused-variable]
 1440 |         struct xfs_da_args              *args = attr->xattri_da_args;
      |                                          ^~~~

Clean this up.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 836ab1b8ed7b..acbd7dbd2281 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1439,12 +1439,11 @@ static int
 xfs_attr_node_try_addname(
 	struct xfs_attr_intent		*attr)
 {
-	struct xfs_da_args		*args = attr->xattri_da_args;
 	struct xfs_da_state		*state = attr->xattri_da_state;
 	struct xfs_da_state_blk		*blk;
 	int				error;
 
-	trace_xfs_attr_node_addname(args);
+	trace_xfs_attr_node_addname(state->args);
 
 	blk = &state->path.blk[state->path.active-1];
 	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);

Re: [PATCH 2/2] xfs: fix variable state usage control knob

[Dave Chinner]

On Sun, Jun 05, 2022 at 09:40:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The variable @args is fed to a tracepoint, and that's the only place
> it's used.  This is fine for the kernel, but for userspace, tracepoints
> are #define'd out of existence, which results in this warning on gcc
> 11.2:
> 
> xfs_attr.c: In function ‘xfs_attr_node_try_addname’:
> xfs_attr.c:1440:42: warning: unused variable ‘args’ [-Wunused-variable]
>  1440 |         struct xfs_da_args              *args = attr->xattri_da_args;
>       |                                          ^~~~
> 
> Clean this up.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_attr.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Yeah, I noticed this in the xfsprogs libxfs port and fixed it there
on the port by converting the xfs_attr3_leaf_add(... state->args)
parameter to use args. This way works too, and the xfsprogs libxfs
can easily be cleaned up in the next xfsprogs libxfs sync.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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

[Dave Chinner]

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

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

[Darrick J. Wong]

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

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

[Dave Chinner]

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