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

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

[Darrick J. Wong]

Hi all,

This short series fixes a couple of places where the deferred xattr
state machine could leak the xfs_da_state structure.  Make log recovery
more robust by checking the op state and attr filter flags of the
recovered log items.

v2: Don't create a new flags namespace for the attr_filter values, since we
    already have one, and add RVB tags.  Fix a crash that Dave found in the
    first patch when node_removename decides to free the da state, but we
    never set the state variable.

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=attr-intent-fixes-5.19
---
 fs/xfs/libxfs/xfs_attr.c       |   32 ++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_log_format.h |   10 +++++++++-
 fs/xfs/xfs_attr_item.c         |   34 ++++++++++++++++++++++++++--------
 3 files changed, 57 insertions(+), 19 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>
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..d0418b79056f 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(attr->xattri_da_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>
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 d0418b79056f..576de34cfca0 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>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
---
 fs/xfs/xfs_attr_item.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 687cf517841a..ae227a56bbed 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -349,6 +349,7 @@ xfs_attr_log_item(
 	 */
 	attrp = &attrip->attri_format;
 	attrp->alfi_ino = attr->xattri_da_args->dp->i_ino;
+	ASSERT(!(attr->xattri_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK));
 	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;
@@ -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* 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);

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

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

[Dave Chinner]

On Wed, May 18, 2022 at 11:54:53AM -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>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_attr_item.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

LGTM.

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

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

[Dave Chinner]

On Wed, May 18, 2022 at 11:54:47AM -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>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Looks OK.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
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 1/4] xfs: don't leak da state when freeing the attr intent item

[Dave Chinner]

On Wed, May 18, 2022 at 11:54:41AM -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>
> 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(-)

Looks ok.

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

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: [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);
> 

[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);