diff mbox series

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

Message ID 165267191771.625255.3595054157709968327.stgit@magnolia (mailing list archive)
State Accepted, archived
Headers show
Series xfs: fix leaks and validation errors in logged xattr updates | expand

Commit Message

Darrick J. Wong May 16, 2022, 3:31 a.m. UTC 
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(-)

Comments

Allison Henderson May 16, 2022, 11:55 p.m. UTC | #1 
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;
>  }
>  
>
diff mbox series

Patch

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