lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:   Fri,  8 Apr 2022 13:15:07 -0400
From:   Sweet Tea Dorminy <sweettea-kernel@...miny.me>
To:     Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, linux-kernel@...r.kernel.org,
        linux-btrfs@...r.kernel.org, kernel-team@...com
Cc:     Sweet Tea Dorminy <sweettea-kernel@...miny.me>,
        Omar Sandoval <osandov@...ndov.com>,
        Naohiro Aota <naohiro.aota@....com>
Subject: [PATCH] btrfs: restore inode creation before xattr setting

According to the tree checker, "all xattrs with a given objectid follow
the inode with that objectid in the tree" is an invariant. This was
broken by the recent change "btrfs: move common inode creation code into
btrfs_create_new_inode()", which moved acl creation and property
inheritance (stored in xattrs) to before inode insertion into the tree.
As a result, under certain timings, the xattrs could be written to the
tree before the inode, causing the tree checker to report violation of
the invariant.

Move property inheritance and acl creation back to their old ordering
after the inode insertion.

Suggested-by: Omar Sandoval <osandov@...ndov.com>
Reported-by: Naohiro Aota <naohiro.aota@....com>
Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@...miny.me>
---
This should apply on top of osandov's patch at
 https://lore.kernel.org/linux-btrfs/da6cfa1b8e42db5c8954680cac1ca322d463b880.1647306546.git.osandov@fb.com/

It's survived a good dose of fstests, and several iterations of specific
tests that were failing, e.g. generic/650.

David: I don't know if you'd rather roll this into osandov's original
patch, or whether you'd like me or osandov to resend the patch linked
above with this addition rolled into it, or whether you'd like to apply
it separately. 

---
 fs/btrfs/inode.c | 74 ++++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 2f4935649555..213e7048d911 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6213,43 +6213,6 @@ int btrfs_create_new_inode(struct btrfs_trans_handle *trans,
 		goto out;
 	}
 
-	if (args->subvol) {
-		struct inode *parent;
-
-		/*
-		 * Subvolumes inherit properties from their parent subvolume,
-		 * not the directory they were created in.
-		 */
-		parent = btrfs_iget(fs_info->sb, BTRFS_FIRST_FREE_OBJECTID,
-				    BTRFS_I(dir)->root);
-		if (IS_ERR(parent)) {
-			ret = PTR_ERR(parent);
-		} else {
-			ret = btrfs_inode_inherit_props(trans, inode, parent);
-			iput(parent);
-		}
-	} else {
-		ret = btrfs_inode_inherit_props(trans, inode, dir);
-	}
-	if (ret) {
-		btrfs_err(fs_info,
-			  "error inheriting props for ino %llu (root %llu): %d",
-			  btrfs_ino(BTRFS_I(inode)), root->root_key.objectid,
-			  ret);
-	}
-
-	/*
-	 * Subvolumes don't inherit ACLs or get passed to the LSM. This is
-	 * probably a bug.
-	 */
-	if (!args->subvol) {
-		ret = btrfs_init_inode_security(trans, args);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			goto discard;
-		}
-	}
-
 	/*
 	 * We could have gotten an inode number from somebody who was fsynced
 	 * and then removed in this same transaction, so let's just set full
@@ -6327,6 +6290,43 @@ int btrfs_create_new_inode(struct btrfs_trans_handle *trans,
 	btrfs_mark_buffer_dirty(path->nodes[0]);
 	btrfs_release_path(path);
 
+	if (args->subvol) {
+		struct inode *parent;
+
+		/*
+		 * Subvolumes inherit properties from their parent subvolume,
+		 * not the directory they were created in.
+		 */
+		parent = btrfs_iget(fs_info->sb, BTRFS_FIRST_FREE_OBJECTID,
+				    BTRFS_I(dir)->root);
+		if (IS_ERR(parent)) {
+			ret = PTR_ERR(parent);
+		} else {
+			ret = btrfs_inode_inherit_props(trans, inode, parent);
+			iput(parent);
+		}
+	} else {
+		ret = btrfs_inode_inherit_props(trans, inode, dir);
+	}
+	if (ret) {
+		btrfs_err(fs_info,
+			  "error inheriting props for ino %llu (root %llu): %d",
+			  btrfs_ino(BTRFS_I(inode)), root->root_key.objectid,
+			  ret);
+	}
+
+	/*
+	 * Subvolumes don't inherit ACLs or get passed to the LSM. This is
+	 * probably a bug.
+	 */
+	if (!args->subvol) {
+		ret = btrfs_init_inode_security(trans, args);
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			goto discard;
+		}
+	}
+
 	inode_tree_add(inode);
 
 	trace_btrfs_inode_new(inode);
-- 
2.35.1

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ