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:
[<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>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

diff --git a/libxfs/defer_item.c b/libxfs/defer_item.c
index 3c864cf41c6fb591eac549fd6a4ef132b241fd97..e88df58fef38793faee370a9e874b44e34fd7dc8 100644 (file)
--- 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 7468e296549ac3de108575ba6a9f9ce39738a43c..a33490eeeced6db1b5eadace00a34e1fcb9a352b 100644 (file)
--- 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;
 }