Re: [PATCH 3/4] xfs: share xattr name and value buffers when logging xattr updates

[Dave Chinner <david@fromorbit.com>]

On Sun, May 22, 2022 at 08:28:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While running xfs/297 and generic/642, I noticed a crash in
> xfs_attri_item_relog when it tries to copy the attr name to the new
> xattri log item.  I think what happened here was that we called
> ->iop_commit on the old attri item (which nulls out the pointers) as
> part of a log force at the same time that a chained attr operation was
> ongoing.  The system was busy enough that at some later point, the defer
> ops operation decided it was necessary to relog the attri log item, but
> as we've detached the name buffer from the old attri log item, we can't
> copy it to the new one, and kaboom.
> 
> I think there's a broader refcounting problem with LARP mode -- the
> setxattr code can return to userspace before the CIL actually formats
> and commits the log item, which results in a UAF bug.  Therefore, the
> xattr log item needs to be able to retain a reference to the name and
> value buffers until the log items have completely cleared the log.
> Furthermore, each time we create an intent log item, we allocate new
> memory and (re)copy the contents; sharing here would be very useful.
> 
> Solve the UAF and the unnecessary memory allocations by having the log
> code create a single refcounted buffer to contain the name and value
> contents.  This buffer can be passed from old to new during a relog
> operation, and the logging code can (optionally) attach it to the
> xfs_attr_item for reuse when LARP mode is enabled.
> 
> This also fixes a problem where the xfs_attri_log_item objects weren't
> being freed back to the same cache where they came from.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_attr.h |    8 +
>  fs/xfs/xfs_attr_item.c   |  271 ++++++++++++++++++++++++++--------------------
>  fs/xfs/xfs_attr_item.h   |   13 ++
>  fs/xfs/xfs_log.h         |    7 +
>  4 files changed, 178 insertions(+), 121 deletions(-)

Lots neater!

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com