Re: [PATCH 1/3] xfs: fix TOCTOU race involving the new logged xattrs control knob

[Dave Chinner <david@fromorbit.com>]

On Tue, Jun 07, 2022 at 02:03:08PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> I found a race involving the larp control knob, aka the debugging knob
> that lets developers enable logging of extended attribute updates:
> 
> Thread 1			Thread 2
> 
> echo 0 > /sys/fs/xfs/debug/larp
> 				setxattr(REPLACE)
> 				xfs_has_larp (returns false)
> 				xfs_attr_set
> 
> echo 1 > /sys/fs/xfs/debug/larp
> 
> 				xfs_attr_defer_replace
> 				xfs_attr_init_replace_state
> 				xfs_has_larp (returns true)
> 				xfs_attr_init_remove_state
> 
> 				<oops, wrong DAS state!>
> 
> This isn't a particularly severe problem right now because xattr logging
> is only enabled when CONFIG_XFS_DEBUG=y, and developers *should* know
> what they're doing.
> 
> However, the eventual intent is that callers should be able to ask for
> the assistance of the log in persisting xattr updates.  This capability
> might not be required for /all/ callers, which means that dynamic
> control must work correctly.  Once an xattr update has decided whether
> or not to use logged xattrs, it needs to stay in that mode until the end
> of the operation regardless of what subsequent parallel operations might
> do.
> 
> Therefore, it is an error to continue sampling xfs_globals.larp once
> xfs_attr_change has made a decision about larp, and it was not correct
> for me to have told Allison that ->create_intent functions can sample
> the global log incompat feature bitfield to decide to elide a log item.
> 
> Instead, create a new op flag for the xfs_da_args structure, and convert
> all other callers of xfs_has_larp and xfs_sb_version_haslogxattrs within
> the attr update state machine to look for the operations flag.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks fine now.

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