From: Darrick J. Wong Date: Wed, 22 Jun 2022 19:28:52 +0000 (-0500) Subject: xfs: don't leak da state when freeing the attr intent item X-Git-Tag: v5.19.0-rc0~25 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5b39118945c6f7d5d5bd95f211b0405cc8d61e2b;p=thirdparty%2Fxfsprogs-dev.git xfs: don't leak da state when freeing the attr intent item Source kernel commit: 309001c22cdd75c62e6c3a217bf6967e178f929a 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: [] xfs_da_state_alloc+0x1a/0x30 [xfs] [] xfs_attr_node_hasname+0x23/0x90 [xfs] [] xfs_attr_set_iter+0x441/0xa30 [xfs] [] xfs_xattri_finish_update+0x44/0x80 [xfs] [] xfs_attr_finish_item+0x1e/0x40 [xfs] [] xfs_defer_finish_noroll+0x184/0x740 [xfs] [] __xfs_trans_commit+0x153/0x3e0 [xfs] [] xfs_attr_set+0x469/0x7e0 [xfs] [] xfs_xattr_set+0x89/0xd0 [xfs] [] __vfs_removexattr+0x52/0x70 [] __vfs_removexattr_locked+0xb8/0x150 [] vfs_removexattr+0x56/0x100 [] removexattr+0x58/0x90 [] path_removexattr+0x9e/0xc0 [] __x64_sys_lremovexattr+0x14/0x20 [] 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 Reviewed-by: Allison Henderson Reviewed-by: Dave Chinner Signed-off-by: Dave Chinner Signed-off-by: Eric Sandeen --- diff --git a/libxfs/defer_item.c b/libxfs/defer_item.c index 3c864cf41..e88df58fe 100644 --- a/libxfs/defer_item.c +++ b/libxfs/defer_item.c @@ -485,6 +485,15 @@ xfs_attr_create_done( return NULL; } +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( @@ -516,7 +525,7 @@ xfs_attr_finish_item( error = -EAGAIN; out: if (error != -EAGAIN) - kmem_free(attr); + xfs_attr_free_item(attr); return error; } @@ -529,7 +538,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); } const struct xfs_defer_op_type xfs_attr_defer_type = { diff --git a/libxfs/xfs_attr.c b/libxfs/xfs_attr.c index 7468e2965..a33490eee 100644 --- a/libxfs/xfs_attr.c +++ b/libxfs/xfs_attr.c @@ -602,26 +602,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; } @@ -1454,8 +1457,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; } @@ -1509,6 +1514,7 @@ xfs_attr_node_try_addname( out: xfs_da_state_free(state); + attr->xattri_da_state = NULL; return error; }