[PATCHSET 0/4] xfs: fix leaks and validation errors in logged xattr updates

[PATCHSET 0/4] xfs: fix leaks and validation errors in logged xattr updates

[Darrick J. Wong]

Hi all,

As I've detailed in my reply to Dave, this is a short series of fixes
for the 5.19 for-next branch that fixes some validation deficiencies in
xattr log item recovery and some memory leaks due to a confusing API.

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

--D
---
 fs/xfs/libxfs/xfs_attr.c       |   32 ++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_log_format.h |    9 ++++++++-
 fs/xfs/xfs_attr_item.c         |   36 +++++++++++++++++++++++++++---------
 3 files changed, 57 insertions(+), 20 deletions(-)

[PATCH 1/4] xfs: don't leak da state when freeing the attr intent item

[Darrick J. Wong]

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

kmemleak reported that we lost an xfs_da_state while removing xattrs in
generic/020:

unreferenced object 0xffff88801c0e4b40 (size 480):
  comm "attr", pid 30515, jiffies 4294931061 (age 5.960s)
  hex dump (first 32 bytes):
    78 bc 65 07 00 c9 ff ff 00 30 60 1c 80 88 ff ff  x.e......0`.....
    02 00 00 00 00 00 00 00 80 18 83 4e 80 88 ff ff  ...........N....
  backtrace:
    [<ffffffffa023ef4a>] xfs_da_state_alloc+0x1a/0x30 [xfs]
    [<ffffffffa021b6f3>] xfs_attr_node_hasname+0x23/0x90 [xfs]
    [<ffffffffa021c6f1>] xfs_attr_set_iter+0x441/0xa30 [xfs]
    [<ffffffffa02b5104>] xfs_xattri_finish_update+0x44/0x80 [xfs]
    [<ffffffffa02b515e>] xfs_attr_finish_item+0x1e/0x40 [xfs]
    [<ffffffffa0244744>] xfs_defer_finish_noroll+0x184/0x740 [xfs]
    [<ffffffffa02a6473>] __xfs_trans_commit+0x153/0x3e0 [xfs]
    [<ffffffffa021d149>] xfs_attr_set+0x469/0x7e0 [xfs]
    [<ffffffffa02a78d9>] xfs_xattr_set+0x89/0xd0 [xfs]
    [<ffffffff812e6512>] __vfs_removexattr+0x52/0x70
    [<ffffffff812e6a08>] __vfs_removexattr_locked+0xb8/0x150
    [<ffffffff812e6af6>] vfs_removexattr+0x56/0x100
    [<ffffffff812e6bf8>] removexattr+0x58/0x90
    [<ffffffff812e6cce>] path_removexattr+0x9e/0xc0
    [<ffffffff812e6d44>] __x64_sys_lremovexattr+0x14/0x20
    [<ffffffff81786b35>] do_syscall_64+0x35/0x80

I think this is a consequence of xfs_attr_node_removename_setup
attaching a new da(btree) state to xfs_attr_item and never freeing it.
I /think/ it's the case that the remove paths could detach the da state
earlier in the remove state machine since nothing else accesses the
state.  However, let's future-proof the new xattr code by adding a
catch-all when we free the xfs_attr_item to make sure we never leak the
da state.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c |   22 ++++++++++++++--------
 fs/xfs/xfs_attr_item.c   |   15 ++++++++++++---
 2 files changed, 26 insertions(+), 11 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 14ae0826bc15..2da24954b2d7 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -604,26 +604,29 @@ int xfs_attr_node_removename_setup(
 	struct xfs_attr_item		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
-	struct xfs_da_state		**state = &attr->xattri_da_state;
+	struct xfs_da_state		*state;
 	int				error;
 
-	error = xfs_attr_node_hasname(args, state);
+	error = xfs_attr_node_hasname(args, &attr->xattri_da_state);
 	if (error != -EEXIST)
 		goto out;
 	error = 0;
 
-	ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL);
-	ASSERT((*state)->path.blk[(*state)->path.active - 1].magic ==
+	state = attr->xattri_da_state;
+	ASSERT(state->path.blk[state->path.active - 1].bp != NULL);
+	ASSERT(state->path.blk[state->path.active - 1].magic ==
 		XFS_ATTR_LEAF_MAGIC);
 
-	error = xfs_attr_leaf_mark_incomplete(args, *state);
+	error = xfs_attr_leaf_mark_incomplete(args, state);
 	if (error)
 		goto out;
 	if (args->rmtblkno > 0)
 		error = xfs_attr_rmtval_invalidate(args);
 out:
-	if (error)
-		xfs_da_state_free(*state);
+	if (error) {
+		xfs_da_state_free(state);
+		attr->xattri_da_state = NULL;
+	}
 
 	return error;
 }
@@ -1456,8 +1459,10 @@ xfs_attr_node_addname_find_attr(
 
 	return 0;
 error:
-	if (attr->xattri_da_state)
+	if (attr->xattri_da_state) {
 		xfs_da_state_free(attr->xattri_da_state);
+		attr->xattri_da_state = NULL;
+	}
 	return error;
 }
 
@@ -1511,6 +1516,7 @@ xfs_attr_node_try_addname(
 
 out:
 	xfs_da_state_free(state);
+	attr->xattri_da_state = NULL;
 	return error;
 }
 
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index e8ac88d9fd14..687cf517841a 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -396,6 +396,15 @@ xfs_attr_create_intent(
 	return &attrip->attri_item;
 }
 
+static inline void
+xfs_attr_free_item(
+	struct xfs_attr_item		*attr)
+{
+	if (attr->xattri_da_state)
+		xfs_da_state_free(attr->xattri_da_state);
+	kmem_free(attr);
+}
+
 /* Process an attr. */
 STATIC int
 xfs_attr_finish_item(
@@ -420,7 +429,7 @@ xfs_attr_finish_item(
 
 	error = xfs_xattri_finish_update(attr, done_item);
 	if (error != -EAGAIN)
-		kmem_free(attr);
+		xfs_attr_free_item(attr);
 
 	return error;
 }
@@ -441,7 +450,7 @@ xfs_attr_cancel_item(
 	struct xfs_attr_item		*attr;
 
 	attr = container_of(item, struct xfs_attr_item, xattri_list);
-	kmem_free(attr);
+	xfs_attr_free_item(attr);
 }
 
 STATIC xfs_lsn_t
@@ -613,7 +622,7 @@ xfs_attri_item_recover(
 	xfs_irele(ip);
 out:
 	if (ret != -EAGAIN)
-		kmem_free(attr);
+		xfs_attr_free_item(attr);
 	return error;
 }
 

[PATCH 2/4] xfs: don't leak the retained da state when doing a leaf to node conversion

[Darrick J. Wong]

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

If a setxattr operation finds an xattr structure in leaf format, adding
the attr can fail due to lack of space and hence requires an upgrade to
node format.  After this happens, we'll roll the transaction and
re-enter the state machine, at which time we need to perform a second
lookup of the attribute name to find its new location.  This lookup
attaches a new da state structure to the xfs_attr_item but doesn't free
the old one (from the leaf lookup) and leaks it.  Fix that.

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


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 2da24954b2d7..499a15480b57 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1401,8 +1401,10 @@ xfs_attr_node_hasname(
 	int			retval, error;
 
 	state = xfs_da_state_alloc(args);
-	if (statep != NULL)
+	if (statep != NULL) {
+		ASSERT(*statep == NULL);
 		*statep = state;
+	}
 
 	/*
 	 * Search to see if name exists, and get back a pointer to it.
@@ -1428,6 +1430,10 @@ xfs_attr_node_addname_find_attr(
 	struct xfs_da_args	*args = attr->xattri_da_args;
 	int			error;
 
+	if (attr->xattri_da_state)
+		xfs_da_state_free(attr->xattri_da_state);
+	attr->xattri_da_state = NULL;
+
 	/*
 	 * Search to see if name already exists, and get back a pointer
 	 * to where it should go.
@@ -1593,7 +1599,7 @@ STATIC int
 xfs_attr_node_get(
 	struct xfs_da_args	*args)
 {
-	struct xfs_da_state	*state;
+	struct xfs_da_state	*state = NULL;
 	struct xfs_da_state_blk	*blk;
 	int			i;
 	int			error;

[PATCH 3/4] xfs: reject unknown xattri log item operation flags during recovery

[Darrick J. Wong]

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

Make sure we screen the op flags field of recovered xattr intent log
items to reject flag bits that we don't know about.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_attr_item.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 687cf517841a..459b6c93b40b 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -349,7 +349,8 @@ xfs_attr_log_item(
 	 */
 	attrp = &attrip->attri_format;
 	attrp->alfi_ino = attr->xattri_da_args->dp->i_ino;
-	attrp->alfi_op_flags = attr->xattri_op_flags;
+	attrp->alfi_op_flags = attr->xattri_op_flags &
+						XFS_ATTR_OP_FLAGS_TYPE_MASK;
 	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
 	attrp->alfi_name_len = attr->xattri_da_args->namelen;
 	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
@@ -496,6 +497,9 @@ xfs_attri_validate(
 	if (attrp->__pad != 0)
 		return false;
 
+	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
+		return false;
+
 	/* alfi_op_flags should be either a set or remove */
 	switch (op) {
 	case XFS_ATTR_OP_FLAGS_SET:
@@ -556,7 +560,8 @@ xfs_attri_item_recover(
 	args = (struct xfs_da_args *)(attr + 1);
 
 	attr->xattri_da_args = args;
-	attr->xattri_op_flags = attrp->alfi_op_flags;
+	attr->xattri_op_flags = attrp->alfi_op_flags &
+						XFS_ATTR_OP_FLAGS_TYPE_MASK;
 
 	args->dp = ip;
 	args->geo = mp->m_attr_geo;
@@ -567,7 +572,7 @@ xfs_attri_item_recover(
 	args->attr_filter = attrp->alfi_attr_flags;
 	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
 
-	switch (attrp->alfi_op_flags & XFS_ATTR_OP_FLAGS_TYPE_MASK) {
+	switch (attr->xattri_op_flags) {
 	case XFS_ATTR_OP_FLAGS_SET:
 	case XFS_ATTR_OP_FLAGS_REPLACE:
 		args->value = attrip->attri_value;

[PATCH 4/4] xfs: reject unknown xattri log item filter flags during recovery

[Darrick J. Wong]

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

Make sure we screen the "attr flags" field of recovered xattr intent log
items to reject flag bits that we don't know about.  This is really the
attr *filter* flags, so rename the field and create properly namespaced
flags to fill it.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_log_format.h |    9 ++++++++-
 fs/xfs/xfs_attr_item.c         |   10 +++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index f7edd1ecf6d9..5017500bfd8b 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -911,6 +911,13 @@ struct xfs_icreate_log {
 #define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
 #define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
 
+#define XFS_ATTRI_FILTER_ROOT		(1u << XFS_ATTR_ROOT_BIT)
+#define XFS_ATTRI_FILTER_SECURE		(1u << XFS_ATTR_SECURE_BIT)
+#define XFS_ATTRI_FILTER_INCOMPLETE	(1u << XFS_ATTR_INCOMPLETE_BIT)
+#define XFS_ATTRI_FILTER_MASK		(XFS_ATTRI_FILTER_ROOT | \
+					 XFS_ATTRI_FILTER_SECURE | \
+					 XFS_ATTRI_FILTER_INCOMPLETE)
+
 /*
  * This is the structure used to lay out an attr log item in the
  * log.
@@ -924,7 +931,7 @@ struct xfs_attri_log_format {
 	uint32_t	alfi_op_flags;	/* marks the op as a set or remove */
 	uint32_t	alfi_name_len;	/* attr name length */
 	uint32_t	alfi_value_len;	/* attr value length */
-	uint32_t	alfi_attr_flags;/* attr flags */
+	uint32_t	alfi_attr_filter;/* attr filter flags */
 };
 
 struct xfs_attrd_log_format {
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 459b6c93b40b..7cbb640d7856 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -353,7 +353,8 @@ xfs_attr_log_item(
 						XFS_ATTR_OP_FLAGS_TYPE_MASK;
 	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
 	attrp->alfi_name_len = attr->xattri_da_args->namelen;
-	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
+	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter &
+						XFS_ATTRI_FILTER_MASK;
 
 	memcpy(attrip->attri_name, attr->xattri_da_args->name,
 	       attr->xattri_da_args->namelen);
@@ -500,6 +501,9 @@ xfs_attri_validate(
 	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
 		return false;
 
+	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
+		return false;
+
 	/* alfi_op_flags should be either a set or remove */
 	switch (op) {
 	case XFS_ATTR_OP_FLAGS_SET:
@@ -569,7 +573,7 @@ xfs_attri_item_recover(
 	args->name = attrip->attri_name;
 	args->namelen = attrp->alfi_name_len;
 	args->hashval = xfs_da_hashname(args->name, args->namelen);
-	args->attr_filter = attrp->alfi_attr_flags;
+	args->attr_filter = attrp->alfi_attr_filter & XFS_ATTRI_FILTER_MASK;
 	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
 
 	switch (attr->xattri_op_flags) {
@@ -658,7 +662,7 @@ xfs_attri_item_relog(
 	new_attrp->alfi_op_flags = old_attrp->alfi_op_flags;
 	new_attrp->alfi_value_len = old_attrp->alfi_value_len;
 	new_attrp->alfi_name_len = old_attrp->alfi_name_len;
-	new_attrp->alfi_attr_flags = old_attrp->alfi_attr_flags;
+	new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
 
 	memcpy(new_attrip->attri_name, old_attrip->attri_name,
 		new_attrip->attri_name_len);

Re: [PATCH 1/4] xfs: don't leak da state when freeing the attr intent item

[Alli]

On Sun, 2022-05-15 at 20:31 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> kmemleak reported that we lost an xfs_da_state while removing xattrs
> in
> generic/020:
> 
> unreferenced object 0xffff88801c0e4b40 (size 480):
>   comm "attr", pid 30515, jiffies 4294931061 (age 5.960s)
>   hex dump (first 32 bytes):
>     78 bc 65 07 00 c9 ff ff 00 30 60 1c 80 88 ff ff  x.e......0`.....
>     02 00 00 00 00 00 00 00 80 18 83 4e 80 88 ff ff  ...........N....
>   backtrace:
>     [<ffffffffa023ef4a>] xfs_da_state_alloc+0x1a/0x30 [xfs]
>     [<ffffffffa021b6f3>] xfs_attr_node_hasname+0x23/0x90 [xfs]
>     [<ffffffffa021c6f1>] xfs_attr_set_iter+0x441/0xa30 [xfs]
>     [<ffffffffa02b5104>] xfs_xattri_finish_update+0x44/0x80 [xfs]
>     [<ffffffffa02b515e>] xfs_attr_finish_item+0x1e/0x40 [xfs]
>     [<ffffffffa0244744>] xfs_defer_finish_noroll+0x184/0x740 [xfs]
>     [<ffffffffa02a6473>] __xfs_trans_commit+0x153/0x3e0 [xfs]
>     [<ffffffffa021d149>] xfs_attr_set+0x469/0x7e0 [xfs]
>     [<ffffffffa02a78d9>] xfs_xattr_set+0x89/0xd0 [xfs]
>     [<ffffffff812e6512>] __vfs_removexattr+0x52/0x70
>     [<ffffffff812e6a08>] __vfs_removexattr_locked+0xb8/0x150
>     [<ffffffff812e6af6>] vfs_removexattr+0x56/0x100
>     [<ffffffff812e6bf8>] removexattr+0x58/0x90
>     [<ffffffff812e6cce>] path_removexattr+0x9e/0xc0
>     [<ffffffff812e6d44>] __x64_sys_lremovexattr+0x14/0x20
>     [<ffffffff81786b35>] do_syscall_64+0x35/0x80
> 
> I think this is a consequence of xfs_attr_node_removename_setup
> attaching a new da(btree) state to xfs_attr_item and never freeing
> it.
> I /think/ it's the case that the remove paths could detach the da
> state
> earlier in the remove state machine since nothing else accesses the
> state.  However, let's future-proof the new xattr code by adding a
> catch-all when we free the xfs_attr_item to make sure we never leak
> the
> da state.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Ok, I think it makes sense.
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_attr.c |   22 ++++++++++++++--------
>  fs/xfs/xfs_attr_item.c   |   15 ++++++++++++---
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 14ae0826bc15..2da24954b2d7 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -604,26 +604,29 @@ int xfs_attr_node_removename_setup(
>  	struct xfs_attr_item		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
> -	struct xfs_da_state		**state = &attr-
> >xattri_da_state;
> +	struct xfs_da_state		*state;
>  	int				error;
>  
> -	error = xfs_attr_node_hasname(args, state);
> +	error = xfs_attr_node_hasname(args, &attr->xattri_da_state);
>  	if (error != -EEXIST)
>  		goto out;
>  	error = 0;
>  
> -	ASSERT((*state)->path.blk[(*state)->path.active - 1].bp !=
> NULL);
> -	ASSERT((*state)->path.blk[(*state)->path.active - 1].magic ==
> +	state = attr->xattri_da_state;
> +	ASSERT(state->path.blk[state->path.active - 1].bp != NULL);
> +	ASSERT(state->path.blk[state->path.active - 1].magic ==
>  		XFS_ATTR_LEAF_MAGIC);
>  
> -	error = xfs_attr_leaf_mark_incomplete(args, *state);
> +	error = xfs_attr_leaf_mark_incomplete(args, state);
>  	if (error)
>  		goto out;
>  	if (args->rmtblkno > 0)
>  		error = xfs_attr_rmtval_invalidate(args);
>  out:
> -	if (error)
> -		xfs_da_state_free(*state);
> +	if (error) {
> +		xfs_da_state_free(state);
> +		attr->xattri_da_state = NULL;
> +	}
>  
>  	return error;
>  }
> @@ -1456,8 +1459,10 @@ xfs_attr_node_addname_find_attr(
>  
>  	return 0;
>  error:
> -	if (attr->xattri_da_state)
> +	if (attr->xattri_da_state) {
>  		xfs_da_state_free(attr->xattri_da_state);
> +		attr->xattri_da_state = NULL;
> +	}
>  	return error;
>  }
>  
> @@ -1511,6 +1516,7 @@ xfs_attr_node_try_addname(
>  
>  out:
>  	xfs_da_state_free(state);
> +	attr->xattri_da_state = NULL;
>  	return error;
>  }
>  
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index e8ac88d9fd14..687cf517841a 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -396,6 +396,15 @@ xfs_attr_create_intent(
>  	return &attrip->attri_item;
>  }
>  
> +static inline void
> +xfs_attr_free_item(
> +	struct xfs_attr_item		*attr)
> +{
> +	if (attr->xattri_da_state)
> +		xfs_da_state_free(attr->xattri_da_state);
> +	kmem_free(attr);
> +}
> +
>  /* Process an attr. */
>  STATIC int
>  xfs_attr_finish_item(
> @@ -420,7 +429,7 @@ xfs_attr_finish_item(
>  
>  	error = xfs_xattri_finish_update(attr, done_item);
>  	if (error != -EAGAIN)
> -		kmem_free(attr);
> +		xfs_attr_free_item(attr);
>  
>  	return error;
>  }
> @@ -441,7 +450,7 @@ xfs_attr_cancel_item(
>  	struct xfs_attr_item		*attr;
>  
>  	attr = container_of(item, struct xfs_attr_item, xattri_list);
> -	kmem_free(attr);
> +	xfs_attr_free_item(attr);
>  }
>  
>  STATIC xfs_lsn_t
> @@ -613,7 +622,7 @@ xfs_attri_item_recover(
>  	xfs_irele(ip);
>  out:
>  	if (ret != -EAGAIN)
> -		kmem_free(attr);
> +		xfs_attr_free_item(attr);
>  	return error;
>  }
>  
> 

Re: [PATCH 2/4] xfs: don't leak the retained da state when doing a leaf to node conversion

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If a setxattr operation finds an xattr structure in leaf format,
> adding
> the attr can fail due to lack of space and hence requires an upgrade
> to
> node format.  After this happens, we'll roll the transaction and
> re-enter the state machine, at which time we need to perform a second
> lookup of the attribute name to find its new location.  This lookup
> attaches a new da state structure to the xfs_attr_item but doesn't
> free
> the old one (from the leaf lookup) and leaks it.  Fix that.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_attr.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 2da24954b2d7..499a15480b57 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1401,8 +1401,10 @@ xfs_attr_node_hasname(
>  	int			retval, error;
>  
>  	state = xfs_da_state_alloc(args);
> -	if (statep != NULL)
> +	if (statep != NULL) {
> +		ASSERT(*statep == NULL);
>  		*statep = state;
> +	}
>  
>  	/*
>  	 * Search to see if name exists, and get back a pointer to it.
> @@ -1428,6 +1430,10 @@ xfs_attr_node_addname_find_attr(
>  	struct xfs_da_args	*args = attr->xattri_da_args;
>  	int			error;
>  
> +	if (attr->xattri_da_state)
> +		xfs_da_state_free(attr->xattri_da_state);
> +	attr->xattri_da_state = NULL;
> +
>  	/*
>  	 * Search to see if name already exists, and get back a pointer
>  	 * to where it should go.
> @@ -1593,7 +1599,7 @@ STATIC int
>  xfs_attr_node_get(
>  	struct xfs_da_args	*args)
>  {
> -	struct xfs_da_state	*state;
> +	struct xfs_da_state	*state = NULL;
>  	struct xfs_da_state_blk	*blk;
>  	int			i;
>  	int			error;
> 

Re: [PATCH 3/4] xfs: reject unknown xattri log item operation flags during recovery

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure we screen the op flags field of recovered xattr intent log
> items to reject flag bits that we don't know about.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Ok, looks good
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/xfs_attr_item.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 687cf517841a..459b6c93b40b 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -349,7 +349,8 @@ xfs_attr_log_item(
>  	 */
>  	attrp = &attrip->attri_format;
>  	attrp->alfi_ino = attr->xattri_da_args->dp->i_ino;
> -	attrp->alfi_op_flags = attr->xattri_op_flags;
> +	attrp->alfi_op_flags = attr->xattri_op_flags &
> +						XFS_ATTR_OP_FLAGS_TYPE_
> MASK;
>  	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
>  	attrp->alfi_name_len = attr->xattri_da_args->namelen;
>  	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
> @@ -496,6 +497,9 @@ xfs_attri_validate(
>  	if (attrp->__pad != 0)
>  		return false;
>  
> +	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
> +		return false;
> +
>  	/* alfi_op_flags should be either a set or remove */
>  	switch (op) {
>  	case XFS_ATTR_OP_FLAGS_SET:
> @@ -556,7 +560,8 @@ xfs_attri_item_recover(
>  	args = (struct xfs_da_args *)(attr + 1);
>  
>  	attr->xattri_da_args = args;
> -	attr->xattri_op_flags = attrp->alfi_op_flags;
> +	attr->xattri_op_flags = attrp->alfi_op_flags &
> +						XFS_ATTR_OP_FLAGS_TYPE_
> MASK;
>  
>  	args->dp = ip;
>  	args->geo = mp->m_attr_geo;
> @@ -567,7 +572,7 @@ xfs_attri_item_recover(
>  	args->attr_filter = attrp->alfi_attr_flags;
>  	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
>  
> -	switch (attrp->alfi_op_flags & XFS_ATTR_OP_FLAGS_TYPE_MASK) {
> +	switch (attr->xattri_op_flags) {
>  	case XFS_ATTR_OP_FLAGS_SET:
>  	case XFS_ATTR_OP_FLAGS_REPLACE:
>  		args->value = attrip->attri_value;
> 

Re: [PATCH 4/4] xfs: reject unknown xattri log item filter flags during recovery

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure we screen the "attr flags" field of recovered xattr intent
> log
> items to reject flag bits that we don't know about.  This is really
> the
> attr *filter* flags, so rename the field and create properly
> namespaced
> flags to fill it.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |    9 ++++++++-
>  fs/xfs/xfs_attr_item.c         |   10 +++++++---
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h
> b/fs/xfs/libxfs/xfs_log_format.h
> index f7edd1ecf6d9..5017500bfd8b 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -911,6 +911,13 @@ struct xfs_icreate_log {
>  #define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
>  #define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
>  
> +#define XFS_ATTRI_FILTER_ROOT		(1u <<
> XFS_ATTR_ROOT_BIT)
> +#define XFS_ATTRI_FILTER_SECURE		(1u <<
> XFS_ATTR_SECURE_BIT)
> +#define XFS_ATTRI_FILTER_INCOMPLETE	(1u << XFS_ATTR_INCOMPLETE_BIT)
> +#define XFS_ATTRI_FILTER_MASK		(XFS_ATTRI_FILTER_ROOT
> | \
> +					 XFS_ATTRI_FILTER_SECURE | \
> +					 XFS_ATTRI_FILTER_INCOMPLETE)
> +
It sounds like your already working on a v2 that doesnt use the new
flag scheme, but other than that, I think it looks ok.  Thanks!

Allison

>  /*
>   * This is the structure used to lay out an attr log item in the
>   * log.
> @@ -924,7 +931,7 @@ struct xfs_attri_log_format {
>  	uint32_t	alfi_op_flags;	/* marks the op as a set or remove */
>  	uint32_t	alfi_name_len;	/* attr name length */
>  	uint32_t	alfi_value_len;	/* attr value length */
> -	uint32_t	alfi_attr_flags;/* attr flags */
> +	uint32_t	alfi_attr_filter;/* attr filter flags */
>  };
>  
>  struct xfs_attrd_log_format {
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 459b6c93b40b..7cbb640d7856 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -353,7 +353,8 @@ xfs_attr_log_item(
>  						XFS_ATTR_OP_FLAGS_TYPE_
> MASK;
>  	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
>  	attrp->alfi_name_len = attr->xattri_da_args->namelen;
> -	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
> +	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter &
> +						XFS_ATTRI_FILTER_MASK;
>  
>  	memcpy(attrip->attri_name, attr->xattri_da_args->name,
>  	       attr->xattri_da_args->namelen);
> @@ -500,6 +501,9 @@ xfs_attri_validate(
>  	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
>  		return false;
>  
> +	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
> +		return false;
> +
>  	/* alfi_op_flags should be either a set or remove */
>  	switch (op) {
>  	case XFS_ATTR_OP_FLAGS_SET:
> @@ -569,7 +573,7 @@ xfs_attri_item_recover(
>  	args->name = attrip->attri_name;
>  	args->namelen = attrp->alfi_name_len;
>  	args->hashval = xfs_da_hashname(args->name, args->namelen);
> -	args->attr_filter = attrp->alfi_attr_flags;
> +	args->attr_filter = attrp->alfi_attr_filter &
> XFS_ATTRI_FILTER_MASK;
>  	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
>  
>  	switch (attr->xattri_op_flags) {
> @@ -658,7 +662,7 @@ xfs_attri_item_relog(
>  	new_attrp->alfi_op_flags = old_attrp->alfi_op_flags;
>  	new_attrp->alfi_value_len = old_attrp->alfi_value_len;
>  	new_attrp->alfi_name_len = old_attrp->alfi_name_len;
> -	new_attrp->alfi_attr_flags = old_attrp->alfi_attr_flags;
> +	new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
>  
>  	memcpy(new_attrip->attri_name, old_attrip->attri_name,
>  		new_attrip->attri_name_len);
> 

Re: [PATCH 4/4] xfs: reject unknown xattri log item filter flags during recovery

[Darrick J. Wong]

On Mon, May 16, 2022 at 04:56:20PM -0700, Alli wrote:
> On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Make sure we screen the "attr flags" field of recovered xattr intent
> > log
> > items to reject flag bits that we don't know about.  This is really
> > the
> > attr *filter* flags, so rename the field and create properly
> > namespaced
> > flags to fill it.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_log_format.h |    9 ++++++++-
> >  fs/xfs/xfs_attr_item.c         |   10 +++++++---
> >  2 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h
> > b/fs/xfs/libxfs/xfs_log_format.h
> > index f7edd1ecf6d9..5017500bfd8b 100644
> > --- a/fs/xfs/libxfs/xfs_log_format.h
> > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > @@ -911,6 +911,13 @@ struct xfs_icreate_log {
> >  #define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
> >  #define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
> >  
> > +#define XFS_ATTRI_FILTER_ROOT		(1u <<
> > XFS_ATTR_ROOT_BIT)
> > +#define XFS_ATTRI_FILTER_SECURE		(1u <<
> > XFS_ATTR_SECURE_BIT)
> > +#define XFS_ATTRI_FILTER_INCOMPLETE	(1u << XFS_ATTR_INCOMPLETE_BIT)
> > +#define XFS_ATTRI_FILTER_MASK		(XFS_ATTRI_FILTER_ROOT
> > | \
> > +					 XFS_ATTRI_FILTER_SECURE | \
> > +					 XFS_ATTRI_FILTER_INCOMPLETE)
> > +
> It sounds like your already working on a v2 that doesnt use the new
> flag scheme, but other than that, I think it looks ok.  Thanks!

Yeah.  The new patch simply defines XFS_ATTRI_FILTER_MASK and reuses the
existing XFS_ATTR_{ROOT,SECURE,INCOMPLETE} flags:

/*
 * alfi_attr_filter captures the state of xfs_da_args.attr_filter, so
 * it should never have any other bits set.
 */
#define XFS_ATTRI_FILTER_MASK		(XFS_ATTR_ROOT | \
					 XFS_ATTR_SECURE | \
					 XFS_ATTR_INCOMPLETE)

--D

> Allison
> 
> >  /*
> >   * This is the structure used to lay out an attr log item in the
> >   * log.
> > @@ -924,7 +931,7 @@ struct xfs_attri_log_format {
> >  	uint32_t	alfi_op_flags;	/* marks the op as a set or remove */
> >  	uint32_t	alfi_name_len;	/* attr name length */
> >  	uint32_t	alfi_value_len;	/* attr value length */
> > -	uint32_t	alfi_attr_flags;/* attr flags */
> > +	uint32_t	alfi_attr_filter;/* attr filter flags */
> >  };
> >  
> >  struct xfs_attrd_log_format {
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 459b6c93b40b..7cbb640d7856 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -353,7 +353,8 @@ xfs_attr_log_item(
> >  						XFS_ATTR_OP_FLAGS_TYPE_
> > MASK;
> >  	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
> >  	attrp->alfi_name_len = attr->xattri_da_args->namelen;
> > -	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
> > +	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter &
> > +						XFS_ATTRI_FILTER_MASK;
> >  
> >  	memcpy(attrip->attri_name, attr->xattri_da_args->name,
> >  	       attr->xattri_da_args->namelen);
> > @@ -500,6 +501,9 @@ xfs_attri_validate(
> >  	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
> >  		return false;
> >  
> > +	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
> > +		return false;
> > +
> >  	/* alfi_op_flags should be either a set or remove */
> >  	switch (op) {
> >  	case XFS_ATTR_OP_FLAGS_SET:
> > @@ -569,7 +573,7 @@ xfs_attri_item_recover(
> >  	args->name = attrip->attri_name;
> >  	args->namelen = attrp->alfi_name_len;
> >  	args->hashval = xfs_da_hashname(args->name, args->namelen);
> > -	args->attr_filter = attrp->alfi_attr_flags;
> > +	args->attr_filter = attrp->alfi_attr_filter &
> > XFS_ATTRI_FILTER_MASK;
> >  	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
> >  
> >  	switch (attr->xattri_op_flags) {
> > @@ -658,7 +662,7 @@ xfs_attri_item_relog(
> >  	new_attrp->alfi_op_flags = old_attrp->alfi_op_flags;
> >  	new_attrp->alfi_value_len = old_attrp->alfi_value_len;
> >  	new_attrp->alfi_name_len = old_attrp->alfi_name_len;
> > -	new_attrp->alfi_attr_flags = old_attrp->alfi_attr_flags;
> > +	new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
> >  
> >  	memcpy(new_attrip->attri_name, old_attrip->attri_name,
> >  		new_attrip->attri_name_len);
> > 
> 

Re: [PATCHSET 0/4] xfs: fix leaks and validation errors in logged xattr updates

[Dave Chinner]

On Sun, May 15, 2022 at 08:31:52PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> As I've detailed in my reply to Dave, this is a short series of fixes
> for the 5.19 for-next branch that fixes some validation deficiencies in
> xattr log item recovery and some memory leaks due to a confusing API.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> ---
>  fs/xfs/libxfs/xfs_attr.c       |   32 ++++++++++++++++++++++----------
>  fs/xfs/libxfs/xfs_log_format.h |    9 ++++++++-
>  fs/xfs/xfs_attr_item.c         |   36 +++++++++++++++++++++++++++---------
>  3 files changed, 57 insertions(+), 20 deletions(-)

Ok, somewhere in your two patchsets there is a new crash bug
freeing da_state structures. It's tripped by Catherine's LARP test
with my mods on top of that.

[17268.337737] BUG: kernel NULL pointer dereference, address: 00000000000000e0
[17268.340269] #PF: supervisor read access in kernel mode
[17268.342032] #PF: error_code(0x0000) - not-present page
[17268.343802] PGD 0 P4D 0 
[17268.344758] Oops: 0000 [#1] PREEMPT SMP
[17268.346093] CPU: 15 PID: 3417611 Comm: mount Not tainted 5.18.0-rc7-dgc+ #1247
[17268.348579] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[17268.351457] RIP: 0010:xfs_da_state_kill_altpath+0x5/0x40
[17268.353290] Code: 00 00 48 c7 c2 88 c4 84 82 48 c7 c6 10 c0 84 82 31 ff e8 99 5f 7c 00 eb c3 e8 a7 b3 80 00 cc cc cc cc cc cc cc 0f 1f 44 00 00 <8b> 97 e0 00 00 00 85 d2 7e 26 48 8d 87 e8 00 00 00 83 ea 01 48 8d
[17268.359442] RSP: 0018:ffffc90006227ba8 EFLAGS: 00010286
[17268.360918] RAX: 00000000ffffffc3 RBX: ffff888802540300 RCX: ffff888802264050
[17268.362913] RDX: 0000000000000000 RSI: ffffc90006227bac RDI: 0000000000000000
[17268.364909] RBP: 0000000000000000 R08: ffff8888031b4380 R09: ffff88880537a708
[17268.366893] R10: ffffc90006227ab8 R11: 0000000000000000 R12: 00000000ffffffc3
[17268.368884] R13: 0000000000000000 R14: ffff888802540358 R15: ffff8888064ea000
[17268.370871] FS:  00007f269a1f6800(0000) GS:ffff88883ed80000(0000) knlGS:0000000000000000
[17268.373133] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[17268.374744] CR2: 00000000000000e0 CR3: 000000080510b004 CR4: 0000000000060ee0
[17268.376743] Call Trace:
[17268.377478]  <TASK>
[17268.378131]  xfs_da_state_free+0xe/0x30
[17268.379362]  xfs_attr_set_iter+0x5f2/0xa30
[17268.380533]  xfs_xattri_finish_update+0x45/0x80
[17268.381931]  xfs_attri_item_recover+0x308/0x4d0
[17268.383208]  xlog_recover_process_intents+0xcc/0x330
[17268.384632]  ? _raw_spin_lock_irqsave+0x17/0x20
[17268.386005]  ? _raw_spin_unlock_irqrestore+0xe/0x30
[17268.387375]  ? __mod_timer+0x205/0x3a0
[17268.388456]  ? xfs_alloc_pagf_init+0x52/0x60
[17268.389669]  xlog_recover_finish+0x13/0x100
[17268.390858]  xfs_log_mount_finish+0x157/0x1e0
[17268.392100]  xfs_mountfs+0x548/0x980
[17268.393125]  ? xfs_filestream_get_parent+0x80/0x80
[17268.394482]  xfs_fs_fill_super+0x487/0x8c0
[17268.395651]  ? xfs_open_devices+0x1e0/0x1e0
[17268.396847]  get_tree_bdev+0x16c/0x270
[17268.397920]  vfs_get_tree+0x1f/0xb0
[17268.398917]  path_mount+0x2b6/0xa80
[17268.399923]  __x64_sys_mount+0x103/0x140
[17268.401040]  do_syscall_64+0x35/0x80
[17268.402064]  entry_SYSCALL_64_after_hwframe+0x44/0xae

I haven't yet dug into which patch introduces the problem, but it is
a new regression with these 10 patches applied. I'll try to
reproduce tomorrow, if it's reproducable I'll bisect.

CHeers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCHSET 0/4] xfs: fix leaks and validation errors in logged xattr updates

[Darrick J. Wong]

On Wed, May 18, 2022 at 07:14:13PM +1000, Dave Chinner wrote:
> On Sun, May 15, 2022 at 08:31:52PM -0700, Darrick J. Wong wrote:
> > Hi all,
> > 
> > As I've detailed in my reply to Dave, this is a short series of fixes
> > for the 5.19 for-next branch that fixes some validation deficiencies in
> > xattr log item recovery and some memory leaks due to a confusing API.
> > 
> > This is an extraordinary way to destroy everything.  Enjoy!
> > Comments and questions are, as always, welcome.
> > 
> > --D
> > ---
> >  fs/xfs/libxfs/xfs_attr.c       |   32 ++++++++++++++++++++++----------
> >  fs/xfs/libxfs/xfs_log_format.h |    9 ++++++++-
> >  fs/xfs/xfs_attr_item.c         |   36 +++++++++++++++++++++++++++---------
> >  3 files changed, 57 insertions(+), 20 deletions(-)
> 
> Ok, somewhere in your two patchsets there is a new crash bug
> freeing da_state structures. It's tripped by Catherine's LARP test
> with my mods on top of that.
> 
> [17268.337737] BUG: kernel NULL pointer dereference, address: 00000000000000e0
> [17268.340269] #PF: supervisor read access in kernel mode
> [17268.342032] #PF: error_code(0x0000) - not-present page
> [17268.343802] PGD 0 P4D 0 
> [17268.344758] Oops: 0000 [#1] PREEMPT SMP
> [17268.346093] CPU: 15 PID: 3417611 Comm: mount Not tainted 5.18.0-rc7-dgc+ #1247
> [17268.348579] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> [17268.351457] RIP: 0010:xfs_da_state_kill_altpath+0x5/0x40
> [17268.353290] Code: 00 00 48 c7 c2 88 c4 84 82 48 c7 c6 10 c0 84 82 31 ff e8 99 5f 7c 00 eb c3 e8 a7 b3 80 00 cc cc cc cc cc cc cc 0f 1f 44 00 00 <8b> 97 e0 00 00 00 85 d2 7e 26 48 8d 87 e8 00 00 00 83 ea 01 48 8d
> [17268.359442] RSP: 0018:ffffc90006227ba8 EFLAGS: 00010286
> [17268.360918] RAX: 00000000ffffffc3 RBX: ffff888802540300 RCX: ffff888802264050
> [17268.362913] RDX: 0000000000000000 RSI: ffffc90006227bac RDI: 0000000000000000
> [17268.364909] RBP: 0000000000000000 R08: ffff8888031b4380 R09: ffff88880537a708
> [17268.366893] R10: ffffc90006227ab8 R11: 0000000000000000 R12: 00000000ffffffc3
> [17268.368884] R13: 0000000000000000 R14: ffff888802540358 R15: ffff8888064ea000
> [17268.370871] FS:  00007f269a1f6800(0000) GS:ffff88883ed80000(0000) knlGS:0000000000000000
> [17268.373133] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [17268.374744] CR2: 00000000000000e0 CR3: 000000080510b004 CR4: 0000000000060ee0
> [17268.376743] Call Trace:
> [17268.377478]  <TASK>
> [17268.378131]  xfs_da_state_free+0xe/0x30
> [17268.379362]  xfs_attr_set_iter+0x5f2/0xa30
> [17268.380533]  xfs_xattri_finish_update+0x45/0x80
> [17268.381931]  xfs_attri_item_recover+0x308/0x4d0
> [17268.383208]  xlog_recover_process_intents+0xcc/0x330
> [17268.384632]  ? _raw_spin_lock_irqsave+0x17/0x20
> [17268.386005]  ? _raw_spin_unlock_irqrestore+0xe/0x30
> [17268.387375]  ? __mod_timer+0x205/0x3a0
> [17268.388456]  ? xfs_alloc_pagf_init+0x52/0x60
> [17268.389669]  xlog_recover_finish+0x13/0x100
> [17268.390858]  xfs_log_mount_finish+0x157/0x1e0
> [17268.392100]  xfs_mountfs+0x548/0x980
> [17268.393125]  ? xfs_filestream_get_parent+0x80/0x80
> [17268.394482]  xfs_fs_fill_super+0x487/0x8c0
> [17268.395651]  ? xfs_open_devices+0x1e0/0x1e0
> [17268.396847]  get_tree_bdev+0x16c/0x270
> [17268.397920]  vfs_get_tree+0x1f/0xb0
> [17268.398917]  path_mount+0x2b6/0xa80
> [17268.399923]  __x64_sys_mount+0x103/0x140
> [17268.401040]  do_syscall_64+0x35/0x80
> [17268.402064]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> I haven't yet dug into which patch introduces the problem, but it is
> a new regression with these 10 patches applied. I'll try to
> reproduce tomorrow, if it's reproducable I'll bisect.

Ah, that's a stupid coding error in the first patch -- if the
xfs_attr_node_hasname call returns error != -EEXIST, we never actually
set the state variable that is then passed to xfs_da_state_free.  I knew
I should've converted that last bit:

	if (error) {
		xfs_da_state_free(attr->xattri_da_state);
		attr->xattri_da_state = NULL;
	}

Frankly, I don't even think all those calls to tear down the
xfs_attr_item's da state are necessary, since it seems silly to keep
cycling them through the allocator when we could just zero them when we
no longer need them, and let the attr intent destructor free the state
once and for all.

--D

> CHeers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 4/4] xfs: reject unknown xattri log item filter flags during recovery

[Alli]

On Wed, 2022-05-18 at 11:54 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure we screen the "attr flags" field of recovered xattr intent
> log
> items to reject flag bits that we don't know about.  This is really
> the
> attr *filter* field from xfs_da_args, so rename the field and create
> a mask to make checking for invalid bits easier.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks ok now
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_log_format.h |   10 +++++++++-
>  fs/xfs/xfs_attr_item.c         |   10 +++++++---
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h
> b/fs/xfs/libxfs/xfs_log_format.h
> index f7edd1ecf6d9..a9d08f3d4682 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -911,6 +911,14 @@ struct xfs_icreate_log {
>  #define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
>  #define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
>  
> +/*
> + * alfi_attr_filter captures the state of xfs_da_args.attr_filter,
> so it should
> + * never have any other bits set.
> + */
> +#define XFS_ATTRI_FILTER_MASK		(XFS_ATTR_ROOT | \
> +					 XFS_ATTR_SECURE | \
> +					 XFS_ATTR_INCOMPLETE)
> +
>  /*
>   * This is the structure used to lay out an attr log item in the
>   * log.
> @@ -924,7 +932,7 @@ struct xfs_attri_log_format {
>  	uint32_t	alfi_op_flags;	/* marks the op as a set or remove */
>  	uint32_t	alfi_name_len;	/* attr name length */
>  	uint32_t	alfi_value_len;	/* attr value length */
> -	uint32_t	alfi_attr_flags;/* attr flags */
> +	uint32_t	alfi_attr_filter;/* attr filter flags */
>  };
>  
>  struct xfs_attrd_log_format {
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index ae227a56bbed..fd0a74f3ef45 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -353,7 +353,8 @@ xfs_attr_log_item(
>  	attrp->alfi_op_flags = attr->xattri_op_flags;
>  	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
>  	attrp->alfi_name_len = attr->xattri_da_args->namelen;
> -	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
> +	ASSERT(!(attr->xattri_da_args->attr_filter &
> ~XFS_ATTRI_FILTER_MASK));
> +	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter;
>  
>  	memcpy(attrip->attri_name, attr->xattri_da_args->name,
>  	       attr->xattri_da_args->namelen);
> @@ -500,6 +501,9 @@ xfs_attri_validate(
>  	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
>  		return false;
>  
> +	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
> +		return false;
> +
>  	/* alfi_op_flags should be either a set or remove */
>  	switch (op) {
>  	case XFS_ATTR_OP_FLAGS_SET:
> @@ -569,7 +573,7 @@ xfs_attri_item_recover(
>  	args->name = attrip->attri_name;
>  	args->namelen = attrp->alfi_name_len;
>  	args->hashval = xfs_da_hashname(args->name, args->namelen);
> -	args->attr_filter = attrp->alfi_attr_flags;
> +	args->attr_filter = attrp->alfi_attr_filter &
> XFS_ATTRI_FILTER_MASK;
>  	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
>  
>  	switch (attr->xattri_op_flags) {
> @@ -658,7 +662,7 @@ xfs_attri_item_relog(
>  	new_attrp->alfi_op_flags = old_attrp->alfi_op_flags;
>  	new_attrp->alfi_value_len = old_attrp->alfi_value_len;
>  	new_attrp->alfi_name_len = old_attrp->alfi_name_len;
> -	new_attrp->alfi_attr_flags = old_attrp->alfi_attr_flags;
> +	new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
>  
>  	memcpy(new_attrip->attri_name, old_attrip->attri_name,
>  		new_attrip->attri_name_len);
> 

Re: [PATCH 4/4] xfs: reject unknown xattri log item filter flags during recovery

[Dave Chinner]

On Wed, May 18, 2022 at 11:54:58AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure we screen the "attr flags" field of recovered xattr intent log
> items to reject flag bits that we don't know about.  This is really the
> attr *filter* field from xfs_da_args, so rename the field and create
> a mask to make checking for invalid bits easier.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |   10 +++++++++-
>  fs/xfs/xfs_attr_item.c         |   10 +++++++---
>  2 files changed, 16 insertions(+), 4 deletions(-)

looks good.

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

[PATCH 4/4] xfs: reject unknown xattri log item filter flags during recovery

[Darrick J. Wong]

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

Make sure we screen the "attr flags" field of recovered xattr intent log
items to reject flag bits that we don't know about.  This is really the
attr *filter* field from xfs_da_args, so rename the field and create
a mask to make checking for invalid bits easier.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_log_format.h |   10 +++++++++-
 fs/xfs/xfs_attr_item.c         |   10 +++++++---
 2 files changed, 16 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index f7edd1ecf6d9..a9d08f3d4682 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -911,6 +911,14 @@ struct xfs_icreate_log {
 #define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
 #define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
 
+/*
+ * alfi_attr_filter captures the state of xfs_da_args.attr_filter, so it should
+ * never have any other bits set.
+ */
+#define XFS_ATTRI_FILTER_MASK		(XFS_ATTR_ROOT | \
+					 XFS_ATTR_SECURE | \
+					 XFS_ATTR_INCOMPLETE)
+
 /*
  * This is the structure used to lay out an attr log item in the
  * log.
@@ -924,7 +932,7 @@ struct xfs_attri_log_format {
 	uint32_t	alfi_op_flags;	/* marks the op as a set or remove */
 	uint32_t	alfi_name_len;	/* attr name length */
 	uint32_t	alfi_value_len;	/* attr value length */
-	uint32_t	alfi_attr_flags;/* attr flags */
+	uint32_t	alfi_attr_filter;/* attr filter flags */
 };
 
 struct xfs_attrd_log_format {
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index ae227a56bbed..fd0a74f3ef45 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -353,7 +353,8 @@ xfs_attr_log_item(
 	attrp->alfi_op_flags = attr->xattri_op_flags;
 	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
 	attrp->alfi_name_len = attr->xattri_da_args->namelen;
-	attrp->alfi_attr_flags = attr->xattri_da_args->attr_filter;
+	ASSERT(!(attr->xattri_da_args->attr_filter & ~XFS_ATTRI_FILTER_MASK));
+	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter;
 
 	memcpy(attrip->attri_name, attr->xattri_da_args->name,
 	       attr->xattri_da_args->namelen);
@@ -500,6 +501,9 @@ xfs_attri_validate(
 	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
 		return false;
 
+	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
+		return false;
+
 	/* alfi_op_flags should be either a set or remove */
 	switch (op) {
 	case XFS_ATTR_OP_FLAGS_SET:
@@ -569,7 +573,7 @@ xfs_attri_item_recover(
 	args->name = attrip->attri_name;
 	args->namelen = attrp->alfi_name_len;
 	args->hashval = xfs_da_hashname(args->name, args->namelen);
-	args->attr_filter = attrp->alfi_attr_flags;
+	args->attr_filter = attrp->alfi_attr_filter & XFS_ATTRI_FILTER_MASK;
 	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
 
 	switch (attr->xattri_op_flags) {
@@ -658,7 +662,7 @@ xfs_attri_item_relog(
 	new_attrp->alfi_op_flags = old_attrp->alfi_op_flags;
 	new_attrp->alfi_value_len = old_attrp->alfi_value_len;
 	new_attrp->alfi_name_len = old_attrp->alfi_name_len;
-	new_attrp->alfi_attr_flags = old_attrp->alfi_attr_flags;
+	new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
 
 	memcpy(new_attrip->attri_name, old_attrip->attri_name,
 		new_attrip->attri_name_len);