Commit 309001c2 authored by Darrick J. Wong's avatar Darrick J. Wong Committed by Dave Chinner

xfs: don't leak da state when freeing the attr intent item

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: default avatarDarrick J. Wong <djwong@kernel.org>
Reviewed-by: default avatarAllison Henderson <allison.henderson@oracle.com>
Reviewed-by: default avatarDave Chinner <dchinner@redhat.com>
Signed-off-by: default avatarDave Chinner <david@fromorbit.com>
parent efd409a4
...@@ -604,26 +604,29 @@ int xfs_attr_node_removename_setup( ...@@ -604,26 +604,29 @@ int xfs_attr_node_removename_setup(
struct xfs_attr_item *attr) struct xfs_attr_item *attr)
{ {
struct xfs_da_args *args = attr->xattri_da_args; 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; int error;
error = xfs_attr_node_hasname(args, state); error = xfs_attr_node_hasname(args, &attr->xattri_da_state);
if (error != -EEXIST) if (error != -EEXIST)
goto out; goto out;
error = 0; error = 0;
ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); state = attr->xattri_da_state;
ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == ASSERT(state->path.blk[state->path.active - 1].bp != NULL);
ASSERT(state->path.blk[state->path.active - 1].magic ==
XFS_ATTR_LEAF_MAGIC); XFS_ATTR_LEAF_MAGIC);
error = xfs_attr_leaf_mark_incomplete(args, *state); error = xfs_attr_leaf_mark_incomplete(args, state);
if (error) if (error)
goto out; goto out;
if (args->rmtblkno > 0) if (args->rmtblkno > 0)
error = xfs_attr_rmtval_invalidate(args); error = xfs_attr_rmtval_invalidate(args);
out: out:
if (error) if (error) {
xfs_da_state_free(*state); xfs_da_state_free(attr->xattri_da_state);
attr->xattri_da_state = NULL;
}
return error; return error;
} }
...@@ -1456,8 +1459,10 @@ xfs_attr_node_addname_find_attr( ...@@ -1456,8 +1459,10 @@ xfs_attr_node_addname_find_attr(
return 0; return 0;
error: error:
if (attr->xattri_da_state) if (attr->xattri_da_state) {
xfs_da_state_free(attr->xattri_da_state); xfs_da_state_free(attr->xattri_da_state);
attr->xattri_da_state = NULL;
}
return error; return error;
} }
...@@ -1511,6 +1516,7 @@ xfs_attr_node_try_addname( ...@@ -1511,6 +1516,7 @@ xfs_attr_node_try_addname(
out: out:
xfs_da_state_free(state); xfs_da_state_free(state);
attr->xattri_da_state = NULL;
return error; return error;
} }
......
...@@ -396,6 +396,15 @@ xfs_attr_create_intent( ...@@ -396,6 +396,15 @@ xfs_attr_create_intent(
return &attrip->attri_item; 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. */ /* Process an attr. */
STATIC int STATIC int
xfs_attr_finish_item( xfs_attr_finish_item(
...@@ -420,7 +429,7 @@ xfs_attr_finish_item( ...@@ -420,7 +429,7 @@ xfs_attr_finish_item(
error = xfs_xattri_finish_update(attr, done_item); error = xfs_xattri_finish_update(attr, done_item);
if (error != -EAGAIN) if (error != -EAGAIN)
kmem_free(attr); xfs_attr_free_item(attr);
return error; return error;
} }
...@@ -441,7 +450,7 @@ xfs_attr_cancel_item( ...@@ -441,7 +450,7 @@ xfs_attr_cancel_item(
struct xfs_attr_item *attr; struct xfs_attr_item *attr;
attr = container_of(item, struct xfs_attr_item, xattri_list); attr = container_of(item, struct xfs_attr_item, xattri_list);
kmem_free(attr); xfs_attr_free_item(attr);
} }
STATIC xfs_lsn_t STATIC xfs_lsn_t
...@@ -613,7 +622,7 @@ xfs_attri_item_recover( ...@@ -613,7 +622,7 @@ xfs_attri_item_recover(
xfs_irele(ip); xfs_irele(ip);
out: out:
if (ret != -EAGAIN) if (ret != -EAGAIN)
kmem_free(attr); xfs_attr_free_item(attr);
return error; return error;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment