xfs: fix TOCTOU race involving the new logged xattrs control knob

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>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 836ab1b8ed7b01d7487bcda57718165d42e3019d..0847b4e162378de915a568a1cb661f0d58dcc552 100644 (file)
--- 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 e329da3e7afa92984d33b753956c4aa9781ffd20..b4a2fc77017e0608b333c177e480addef844a48a 100644 (file)
--- 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 15a99040946345ace573fb3eeaa706220f1fb626..37e7c33f62839929d73c43b0ce330454e50b4eaf 100644 (file)
--- 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 d33b7686a0b3aa0c1116ba24b9bf8d3d11fa873a..ffa3df5b2893f71cf864113d3a2704d7ff9d5dc2 100644 (file)
--- 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 4a28c2d770700610fc796e9248a3cc73a14cafb2..135d441334773c18abf238ce12f02cef105aae25 100644 (file)
--- 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 35e13e125ec6a8ecb85b1c62c497635127ac1888..c325a28b89a8d4bd87f8a178300b92b7badd540a 100644 (file)
--- 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;
        }