* Re: [PATCH 1/3] xfs: fix TOCTOU race involving the new logged xattrs control knob
2022-06-07 21:03 ` [PATCH 1/3] xfs: fix TOCTOU race involving the new logged xattrs control knob Darrick J. Wong
@ 2022-06-15 18:13 ` Darrick J. Wong
2022-06-15 22:31 ` Alli
2022-06-19 23:52 ` Dave Chinner
2 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2022-06-15 18:13 UTC (permalink / raw)
To: linux-xfs, david, allison.henderson, chandan.babu
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>
Ping? This has now missed -rc2, and I would like to get this merged for
-rc3.
--D
> ---
> fs/xfs/libxfs/xfs_attr.c | 6 ++++--
> fs/xfs/libxfs/xfs_attr.h | 12 +-----------
> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> fs/xfs/libxfs/xfs_da_btree.h | 4 +++-
> fs/xfs/xfs_attr_item.c | 15 +++++++++------
> fs/xfs/xfs_xattr.c | 17 ++++++++++++++++-
> 6 files changed, 34 insertions(+), 22 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 836ab1b8ed7b..0847b4e16237 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -997,9 +997,11 @@ xfs_attr_set(
> /*
> * We have no control over the attribute names that userspace passes us
> * to remove, so we have to allow the name lookup prior to attribute
> - * removal to fail as well.
> + * removal to fail as well. Preserve the logged flag, since we need
> + * to pass that through to the logging code.
> */
> - args->op_flags = XFS_DA_OP_OKNOENT;
> + args->op_flags = XFS_DA_OP_OKNOENT |
> + (args->op_flags & XFS_DA_OP_LOGGED);
>
> if (args->value) {
> XFS_STATS_INC(mp, xs_attr_set);
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index e329da3e7afa..b4a2fc77017e 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -28,16 +28,6 @@ struct xfs_attr_list_context;
> */
> #define ATTR_MAX_VALUELEN (64*1024) /* max length of a value */
>
> -static inline bool xfs_has_larp(struct xfs_mount *mp)
> -{
> -#ifdef DEBUG
> - /* Logged xattrs require a V5 super for log_incompat */
> - return xfs_has_crc(mp) && xfs_globals.larp;
> -#else
> - return false;
> -#endif
> -}
> -
> /*
> * Kernel-internal version of the attrlist cursor.
> */
> @@ -624,7 +614,7 @@ static inline enum xfs_delattr_state
> xfs_attr_init_replace_state(struct xfs_da_args *args)
> {
> args->op_flags |= XFS_DA_OP_ADDNAME | XFS_DA_OP_REPLACE;
> - if (xfs_has_larp(args->dp->i_mount))
> + if (args->op_flags & XFS_DA_OP_LOGGED)
> return xfs_attr_init_remove_state(args);
> return xfs_attr_init_add_state(args);
> }
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 15a990409463..37e7c33f6283 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1530,7 +1530,7 @@ xfs_attr3_leaf_add_work(
> if (tmp)
> entry->flags |= XFS_ATTR_LOCAL;
> if (args->op_flags & XFS_DA_OP_REPLACE) {
> - if (!xfs_has_larp(mp))
> + if (!(args->op_flags & XFS_DA_OP_LOGGED))
> entry->flags |= XFS_ATTR_INCOMPLETE;
> if ((args->blkno2 == args->blkno) &&
> (args->index2 <= args->index)) {
> diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
> index d33b7686a0b3..ffa3df5b2893 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.h
> +++ b/fs/xfs/libxfs/xfs_da_btree.h
> @@ -92,6 +92,7 @@ typedef struct xfs_da_args {
> #define XFS_DA_OP_NOTIME (1u << 5) /* don't update inode timestamps */
> #define XFS_DA_OP_REMOVE (1u << 6) /* this is a remove operation */
> #define XFS_DA_OP_RECOVERY (1u << 7) /* Log recovery operation */
> +#define XFS_DA_OP_LOGGED (1u << 8) /* Use intent items to track op */
>
> #define XFS_DA_OP_FLAGS \
> { XFS_DA_OP_JUSTCHECK, "JUSTCHECK" }, \
> @@ -101,7 +102,8 @@ typedef struct xfs_da_args {
> { XFS_DA_OP_CILOOKUP, "CILOOKUP" }, \
> { XFS_DA_OP_NOTIME, "NOTIME" }, \
> { XFS_DA_OP_REMOVE, "REMOVE" }, \
> - { XFS_DA_OP_RECOVERY, "RECOVERY" }
> + { XFS_DA_OP_RECOVERY, "RECOVERY" }, \
> + { XFS_DA_OP_LOGGED, "LOGGED" }
>
> /*
> * Storage for holding state during Btree searches and split/join ops.
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 4a28c2d77070..135d44133477 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -413,18 +413,20 @@ xfs_attr_create_intent(
> struct xfs_mount *mp = tp->t_mountp;
> struct xfs_attri_log_item *attrip;
> struct xfs_attr_intent *attr;
> + struct xfs_da_args *args;
>
> ASSERT(count == 1);
>
> - if (!xfs_sb_version_haslogxattrs(&mp->m_sb))
> - return NULL;
> -
> /*
> * Each attr item only performs one attribute operation at a time, so
> * this is a list of one
> */
> attr = list_first_entry_or_null(items, struct xfs_attr_intent,
> xattri_list);
> + args = attr->xattri_da_args;
> +
> + if (!(args->op_flags & XFS_DA_OP_LOGGED))
> + return NULL;
>
> /*
> * Create a buffer to store the attribute name and value. This buffer
> @@ -432,8 +434,6 @@ xfs_attr_create_intent(
> * and the lower level xattr log items.
> */
> if (!attr->xattri_nameval) {
> - struct xfs_da_args *args = attr->xattri_da_args;
> -
> /*
> * Transfer our reference to the name/value buffer to the
> * deferred work state structure.
> @@ -617,7 +617,10 @@ xfs_attri_item_recover(
> args->namelen = nv->name.i_len;
> args->hashval = xfs_da_hashname(args->name, args->namelen);
> args->attr_filter = attrp->alfi_attr_filter & XFS_ATTRI_FILTER_MASK;
> - args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
> + args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT |
> + XFS_DA_OP_LOGGED;
> +
> + ASSERT(xfs_sb_version_haslogxattrs(&mp->m_sb));
>
> switch (attr->xattri_op_flags) {
> case XFS_ATTRI_OP_FLAGS_SET:
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 35e13e125ec6..c325a28b89a8 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -68,6 +68,18 @@ xfs_attr_rele_log_assist(
> xlog_drop_incompat_feat(mp->m_log);
> }
>
> +static inline bool
> +xfs_attr_want_log_assist(
> + struct xfs_mount *mp)
> +{
> +#ifdef DEBUG
> + /* Logged xattrs require a V5 super for log_incompat */
> + return xfs_has_crc(mp) && xfs_globals.larp;
> +#else
> + return false;
> +#endif
> +}
> +
> /*
> * Set or remove an xattr, having grabbed the appropriate logging resources
> * prior to calling libxfs.
> @@ -80,11 +92,14 @@ xfs_attr_change(
> bool use_logging = false;
> int error;
>
> - if (xfs_has_larp(mp)) {
> + ASSERT(!(args->op_flags & XFS_DA_OP_LOGGED));
> +
> + if (xfs_attr_want_log_assist(mp)) {
> error = xfs_attr_grab_log_assist(mp);
> if (error)
> return error;
>
> + args->op_flags |= XFS_DA_OP_LOGGED;
> use_logging = true;
> }
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] xfs: fix TOCTOU race involving the new logged xattrs control knob
2022-06-07 21:03 ` [PATCH 1/3] xfs: fix TOCTOU race involving the new logged xattrs control knob Darrick J. Wong
2022-06-15 18:13 ` Darrick J. Wong
@ 2022-06-15 22:31 ` Alli
2022-06-19 23:52 ` Dave Chinner
2 siblings, 0 replies; 9+ messages in thread
From: Alli @ 2022-06-15 22:31 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, david, chandan.babu
On Tue, 2022-06-07 at 14:03 -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 ok to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
> fs/xfs/libxfs/xfs_attr.c | 6 ++++--
> fs/xfs/libxfs/xfs_attr.h | 12 +-----------
> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> fs/xfs/libxfs/xfs_da_btree.h | 4 +++-
> fs/xfs/xfs_attr_item.c | 15 +++++++++------
> fs/xfs/xfs_xattr.c | 17 ++++++++++++++++-
> 6 files changed, 34 insertions(+), 22 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 836ab1b8ed7b..0847b4e16237 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -997,9 +997,11 @@ xfs_attr_set(
> /*
> * We have no control over the attribute names that userspace
> passes us
> * to remove, so we have to allow the name lookup prior to
> attribute
> - * removal to fail as well.
> + * removal to fail as well. Preserve the logged flag, since we
> need
> + * to pass that through to the logging code.
> */
> - args->op_flags = XFS_DA_OP_OKNOENT;
> + args->op_flags = XFS_DA_OP_OKNOENT |
> + (args->op_flags &
> XFS_DA_OP_LOGGED);
>
> if (args->value) {
> XFS_STATS_INC(mp, xs_attr_set);
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index e329da3e7afa..b4a2fc77017e 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -28,16 +28,6 @@ struct xfs_attr_list_context;
> */
> #define ATTR_MAX_VALUELEN (64*1024) /* max length of a
> value */
>
> -static inline bool xfs_has_larp(struct xfs_mount *mp)
> -{
> -#ifdef DEBUG
> - /* Logged xattrs require a V5 super for log_incompat */
> - return xfs_has_crc(mp) && xfs_globals.larp;
> -#else
> - return false;
> -#endif
> -}
> -
> /*
> * Kernel-internal version of the attrlist cursor.
> */
> @@ -624,7 +614,7 @@ static inline enum xfs_delattr_state
> xfs_attr_init_replace_state(struct xfs_da_args *args)
> {
> args->op_flags |= XFS_DA_OP_ADDNAME | XFS_DA_OP_REPLACE;
> - if (xfs_has_larp(args->dp->i_mount))
> + if (args->op_flags & XFS_DA_OP_LOGGED)
> return xfs_attr_init_remove_state(args);
> return xfs_attr_init_add_state(args);
> }
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c
> b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 15a990409463..37e7c33f6283 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1530,7 +1530,7 @@ xfs_attr3_leaf_add_work(
> if (tmp)
> entry->flags |= XFS_ATTR_LOCAL;
> if (args->op_flags & XFS_DA_OP_REPLACE) {
> - if (!xfs_has_larp(mp))
> + if (!(args->op_flags & XFS_DA_OP_LOGGED))
> entry->flags |= XFS_ATTR_INCOMPLETE;
> if ((args->blkno2 == args->blkno) &&
> (args->index2 <= args->index)) {
> diff --git a/fs/xfs/libxfs/xfs_da_btree.h
> b/fs/xfs/libxfs/xfs_da_btree.h
> index d33b7686a0b3..ffa3df5b2893 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.h
> +++ b/fs/xfs/libxfs/xfs_da_btree.h
> @@ -92,6 +92,7 @@ typedef struct xfs_da_args {
> #define XFS_DA_OP_NOTIME (1u << 5) /* don't update inode
> timestamps */
> #define XFS_DA_OP_REMOVE (1u << 6) /* this is a remove operation
> */
> #define XFS_DA_OP_RECOVERY (1u << 7) /* Log recovery operation */
> +#define XFS_DA_OP_LOGGED (1u << 8) /* Use intent items to track
> op */
>
> #define XFS_DA_OP_FLAGS \
> { XFS_DA_OP_JUSTCHECK, "JUSTCHECK" }, \
> @@ -101,7 +102,8 @@ typedef struct xfs_da_args {
> { XFS_DA_OP_CILOOKUP, "CILOOKUP" }, \
> { XFS_DA_OP_NOTIME, "NOTIME" }, \
> { XFS_DA_OP_REMOVE, "REMOVE" }, \
> - { XFS_DA_OP_RECOVERY, "RECOVERY" }
> + { XFS_DA_OP_RECOVERY, "RECOVERY" }, \
> + { XFS_DA_OP_LOGGED, "LOGGED" }
>
> /*
> * Storage for holding state during Btree searches and split/join
> ops.
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 4a28c2d77070..135d44133477 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -413,18 +413,20 @@ xfs_attr_create_intent(
> struct xfs_mount *mp = tp->t_mountp;
> struct xfs_attri_log_item *attrip;
> struct xfs_attr_intent *attr;
> + struct xfs_da_args *args;
>
> ASSERT(count == 1);
>
> - if (!xfs_sb_version_haslogxattrs(&mp->m_sb))
> - return NULL;
> -
> /*
> * Each attr item only performs one attribute operation at a
> time, so
> * this is a list of one
> */
> attr = list_first_entry_or_null(items, struct xfs_attr_intent,
> xattri_list);
> + args = attr->xattri_da_args;
> +
> + if (!(args->op_flags & XFS_DA_OP_LOGGED))
> + return NULL;
>
> /*
> * Create a buffer to store the attribute name and value. This
> buffer
> @@ -432,8 +434,6 @@ xfs_attr_create_intent(
> * and the lower level xattr log items.
> */
> if (!attr->xattri_nameval) {
> - struct xfs_da_args *args = attr->xattri_da_args;
> -
> /*
> * Transfer our reference to the name/value buffer to
> the
> * deferred work state structure.
> @@ -617,7 +617,10 @@ xfs_attri_item_recover(
> args->namelen = nv->name.i_len;
> args->hashval = xfs_da_hashname(args->name, args->namelen);
> args->attr_filter = attrp->alfi_attr_filter &
> XFS_ATTRI_FILTER_MASK;
> - args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
> + args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT |
> + XFS_DA_OP_LOGGED;
> +
> + ASSERT(xfs_sb_version_haslogxattrs(&mp->m_sb));
>
> switch (attr->xattri_op_flags) {
> case XFS_ATTRI_OP_FLAGS_SET:
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 35e13e125ec6..c325a28b89a8 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -68,6 +68,18 @@ xfs_attr_rele_log_assist(
> xlog_drop_incompat_feat(mp->m_log);
> }
>
> +static inline bool
> +xfs_attr_want_log_assist(
> + struct xfs_mount *mp)
> +{
> +#ifdef DEBUG
> + /* Logged xattrs require a V5 super for log_incompat */
> + return xfs_has_crc(mp) && xfs_globals.larp;
> +#else
> + return false;
> +#endif
> +}
> +
> /*
> * Set or remove an xattr, having grabbed the appropriate logging
> resources
> * prior to calling libxfs.
> @@ -80,11 +92,14 @@ xfs_attr_change(
> bool use_logging = false;
> int error;
>
> - if (xfs_has_larp(mp)) {
> + ASSERT(!(args->op_flags & XFS_DA_OP_LOGGED));
> +
> + if (xfs_attr_want_log_assist(mp)) {
> error = xfs_attr_grab_log_assist(mp);
> if (error)
> return error;
>
> + args->op_flags |= XFS_DA_OP_LOGGED;
> use_logging = true;
> }
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] xfs: fix TOCTOU race involving the new logged xattrs control knob
2022-06-07 21:03 ` [PATCH 1/3] xfs: fix TOCTOU race involving the new logged xattrs control knob Darrick J. Wong
2022-06-15 18:13 ` Darrick J. Wong
2022-06-15 22:31 ` Alli
@ 2022-06-19 23:52 ` Dave Chinner
2 siblings, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2022-06-19 23:52 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, allison.henderson, chandan.babu
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
^ permalink raw reply [flat|nested] 9+ messages in thread