[PATCHSET v2 0/3] xfs: random fixes for 5.19-rc2

[PATCHSET v2 0/3] xfs: random fixes for 5.19-rc2

[Darrick J. Wong]

Hi all,

Here's a quick rollup of all the fixes I've queued for the new LARP and
NREXT64 features.  This time you all get a nice patchset instead of
hand-mailed patches.

v2: Fix a bug with XFS_DA_OP_LOGGED masking, don't get ambitious with
#idef cleanups.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-fixes-5.19
---
 fs/xfs/libxfs/xfs_attr.c      |    9 +++++----
 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_ioctl.c            |    3 ++-
 fs/xfs/xfs_xattr.c            |   17 ++++++++++++++++-
 7 files changed, 37 insertions(+), 25 deletions(-)

[PATCH 1/3] 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.c      |    6 ++++--
 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 ++++++++++++++++-
 6 files changed, 34 insertions(+), 22 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 836ab1b8ed7b..0847b4e16237 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -997,9 +997,11 @@ xfs_attr_set(
 	/*
 	 * We have no control over the attribute names that userspace passes us
 	 * to remove, so we have to allow the name lookup prior to attribute
-	 * removal to fail as well.
+	 * removal to fail as well.  Preserve the logged flag, since we need
+	 * to pass that through to the logging code.
 	 */
-	args->op_flags = XFS_DA_OP_OKNOENT;
+	args->op_flags = XFS_DA_OP_OKNOENT |
+					(args->op_flags & XFS_DA_OP_LOGGED);
 
 	if (args->value) {
 		XFS_STATS_INC(mp, xs_attr_set);
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..ffa3df5b2893 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..c325a28b89a8 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);
 }
 
+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
+}
+
 /*
  * 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/3] xfs: fix variable state usage

[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>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 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 0847b4e16237..1824f61621a2 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1441,12 +1441,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);

[PATCH 3/3] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Darrick J. Wong]

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

It is vitally important that we preserve the state of the NREXT64 inode
flag when we're changing the other flags2 fields.

Fixes: 9b7d16e34bbe ("xfs: Introduce XFS_DIFLAG2_NREXT64 and associated helpers")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>
---
 fs/xfs/xfs_ioctl.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5a364a7d58fd..0d67ff8a8961 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1096,7 +1096,8 @@ xfs_flags2diflags2(
 {
 	uint64_t		di_flags2 =
 		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
-				   XFS_DIFLAG2_BIGTIME));
+				   XFS_DIFLAG2_BIGTIME |
+				   XFS_DIFLAG2_NREXT64));
 
 	if (xflags & FS_XFLAG_DAX)
 		di_flags2 |= XFS_DIFLAG2_DAX;

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

[Darrick J. Wong]

On Tue, Jun 07, 2022 at 02:03:08PM -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.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Ping?  This has now missed -rc2, and I would like to get this merged for
-rc3.

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c      |    6 ++++--
>  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 ++++++++++++++++-
>  6 files changed, 34 insertions(+), 22 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 836ab1b8ed7b..0847b4e16237 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -997,9 +997,11 @@ xfs_attr_set(
>  	/*
>  	 * We have no control over the attribute names that userspace passes us
>  	 * to remove, so we have to allow the name lookup prior to attribute
> -	 * removal to fail as well.
> +	 * removal to fail as well.  Preserve the logged flag, since we need
> +	 * to pass that through to the logging code.
>  	 */
> -	args->op_flags = XFS_DA_OP_OKNOENT;
> +	args->op_flags = XFS_DA_OP_OKNOENT |
> +					(args->op_flags & XFS_DA_OP_LOGGED);
>  
>  	if (args->value) {
>  		XFS_STATS_INC(mp, xs_attr_set);
> 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..ffa3df5b2893 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..c325a28b89a8 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);
>  }
>  
> +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
> +}
> +
>  /*
>   * 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;
>  	}
>  
> 

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

[Alli]

On Tue, 2022-06-07 at 14:03 -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.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks ok to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_attr.c      |    6 ++++--
>  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 ++++++++++++++++-
>  6 files changed, 34 insertions(+), 22 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 836ab1b8ed7b..0847b4e16237 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -997,9 +997,11 @@ xfs_attr_set(
>  	/*
>  	 * We have no control over the attribute names that userspace
> passes us
>  	 * to remove, so we have to allow the name lookup prior to
> attribute
> -	 * removal to fail as well.
> +	 * removal to fail as well.  Preserve the logged flag, since we
> need
> +	 * to pass that through to the logging code.
>  	 */
> -	args->op_flags = XFS_DA_OP_OKNOENT;
> +	args->op_flags = XFS_DA_OP_OKNOENT |
> +					(args->op_flags &
> XFS_DA_OP_LOGGED);
>  
>  	if (args->value) {
>  		XFS_STATS_INC(mp, xs_attr_set);
> 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..ffa3df5b2893 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..c325a28b89a8 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);
>  }
>  
> +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
> +}
> +
>  /*
>   * 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;
>  	}
>  
> 

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

[Alli]

On Tue, 2022-06-07 at 14:03 -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>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  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 0847b4e16237..1824f61621a2 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1441,12 +1441,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 3/3] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Alli]

On Tue, 2022-06-07 at 14:03 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> It is vitally important that we preserve the state of the NREXT64
> inode
> flag when we're changing the other flags2 fields.
> 
> Fixes: 9b7d16e34bbe ("xfs: Introduce XFS_DIFLAG2_NREXT64 and
> associated helpers")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a364a7d58fd..0d67ff8a8961 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1096,7 +1096,8 @@ xfs_flags2diflags2(
>  {
>  	uint64_t		di_flags2 =
>  		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
> -				   XFS_DIFLAG2_BIGTIME));
> +				   XFS_DIFLAG2_BIGTIME |
> +				   XFS_DIFLAG2_NREXT64));
>  
>  	if (xflags & FS_XFLAG_DAX)
>  		di_flags2 |= XFS_DIFLAG2_DAX;
> 

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

[Dave Chinner]

On Tue, Jun 07, 2022 at 02:03:08PM -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.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks fine now.

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