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

[Dave Chinner <david@fromorbit.com>]

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