[PATCH 02/38] xfs: fix dirty transaction cancellation in xfs_attr_set

[Dave Chinner <dgc@kernel.org>]

xfs_attr_set() calls xfs_iext_count_extend() before xfs_attr_lookup().
If xfs_iext_count_extend() upgrades the inode to NREXT64 format it
will dirty the transaction. If the subsequent xfs_attr_lookup() finds
a result that is incompatible with the requested operation (e.g.
EEXIST for CREATE, ENOATTR for REMOVE/REPLACE), the function returns
the error and the caller cancels the transaction. Cancelling a dirty
transaction triggers a filesystem shutdown.

Fix this by moving the xfs_attr_lookup() call before
xfs_iext_count_extend() and validating the lookup result against the
requested operation before dirtying the transaction. The code is
restructured to separate validation from execution: first filter out
the error cases that should cancel cleanly, then extend the extent
count, then dispatch to the appropriate modification function based
on the operation type.

Fixes: 4f86bb4b66c9 ("xfs: Conditionally upgrade existing inodes to use large extent counters")
Assisted-by: LLM
Signed-off-by: Dave Chinner <dgc@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c | 70 ++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index b3f7b2c34ad7..bf0e867628e7 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1136,7 +1136,7 @@ xfs_attr_set(
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_trans_res	tres;
-	int			error, local;
+	int			error, lookup_result, local;
 	int			rmt_blks = 0;
 	unsigned int		total = 0;
 
@@ -1185,48 +1185,48 @@ xfs_attr_set(
 	if (error)
 		return error;
 
-	if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) {
-		error = xfs_iext_count_extend(args->trans, dp, XFS_ATTR_FORK,
-				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
-		if (error)
-			goto out_trans_cancel;
-	}
-
-	error = xfs_attr_lookup(args);
-	switch (error) {
-	case -EEXIST:
-		if (op == XFS_ATTRUPDATE_REMOVE) {
-			/* if no value, we are performing a remove operation */
-			error = xfs_attr_removename(args);
-			if (error)
-				goto out_trans_cancel;
-			break;
-		}
-
-		/* Pure create fails if the attr already exists */
+	/*
+	 * Look up the attr before extending the extent count so that we
+	 * don't dirty the transaction if the op is going to fail with an
+	 * error. Cancelling a dirty transaction would shutdown the fs.
+	 */
+	lookup_result = xfs_attr_lookup(args);
+	if (lookup_result == -EEXIST) {
 		if (op == XFS_ATTRUPDATE_CREATE)
 			goto out_trans_cancel;
-
-		error = xfs_attr_replacename(args, rmt_blks);
-		if (error)
-			goto out_trans_cancel;
-		break;
-	case -ENOATTR:
-		/* Can't remove what isn't there. */
-		if (op == XFS_ATTRUPDATE_REMOVE)
+	} else if (lookup_result == -ENOATTR) {
+		if (op == XFS_ATTRUPDATE_REMOVE ||
+		    op == XFS_ATTRUPDATE_REPLACE)
 			goto out_trans_cancel;
+	} else {
+		error = lookup_result;
+		goto out_trans_cancel;
+	}
 
-		/* Pure replace fails if no existing attr to replace. */
-		if (op == XFS_ATTRUPDATE_REPLACE)
-			goto out_trans_cancel;
+	error = xfs_iext_count_extend(args->trans, dp, XFS_ATTR_FORK,
+			XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
+	if (error)
+		goto out_trans_cancel;
 
+	switch (op) {
+	case XFS_ATTRUPDATE_REMOVE:
+		error = xfs_attr_removename(args);
+		break;
+	case XFS_ATTRUPDATE_CREATE:
 		error = xfs_attr_setname(args, rmt_blks);
-		if (error)
-			goto out_trans_cancel;
 		break;
-	default:
-		goto out_trans_cancel;
+	case XFS_ATTRUPDATE_UPSERT:
+		if (lookup_result == -ENOATTR) {
+			error = xfs_attr_setname(args, rmt_blks);
+			break;
+		}
+		fallthrough;
+	case XFS_ATTRUPDATE_REPLACE:
+		error = xfs_attr_replacename(args, rmt_blks);
+		break;
 	}
+	if (error)
+		goto out_trans_cancel;
 
 	/*
 	 * If this is a synchronous mount, make sure that the
-- 
2.55.0