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]
Message-Id: <20240805201241.27286-1-jack@suse.cz>
Date: Mon,  5 Aug 2024 22:12:41 +0200
From: Jan Kara <jack@...e.cz>
To: Ted Tso <tytso@....edu>
Cc: <linux-ext4@...r.kernel.org>,
	Christian Brauner <brauner@...nel.org>,
	<linux-fsdevel@...r.kernel.org>,
	Jan Kara <jack@...e.cz>
Subject: [PATCH] ext4: don't set SB_RDONLY after filesystem errors

When the filesystem is mounted with errors=remount-ro, we were setting
SB_RDONLY flag to stop all filesystem modifications. We knew this misses
proper locking (sb->s_umount) and does not go through proper filesystem
remount procedure but it has been the way this worked since early ext2
days and it was good enough for catastrophic situation damage
mitigation. Recently, syzbot has found a way (see link) to trigger
warnings in filesystem freezing because the code got confused by
SB_RDONLY changing under its hands. Since these days we set
EXT4_FLAGS_SHUTDOWN on the superblock which is enough to stop all
filesystem modifications, modifying SB_RDONLY shouldn't be needed. So
stop doing that.

Link: https://lore.kernel.org/all/000000000000b90a8e061e21d12f@google.com
Reported-by: Christian Brauner <brauner@...nel.org>
Signed-off-by: Jan Kara <jack@...e.cz>
---
 fs/ext4/super.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Note that this patch introduces fstests failure with generic/459 test because
it assumes that either freezing succeeds or 'ro' is among mount options. But
we fail the freeze with EFSCORRUPTED. This needs fixing in the test but at this
point I'm not sure how exactly.

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e72145c4ae5a..93c016b186c0 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -735,11 +735,12 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
 
 	ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
 	/*
-	 * Make sure updated value of ->s_mount_flags will be visible before
-	 * ->s_flags update
+	 * EXT4_FLAGS_SHUTDOWN was set which stops all filesystem
+	 * modifications. We don't set SB_RDONLY because that requires
+	 * sb->s_umount semaphore and setting it without proper remount
+	 * procedure is confusing code such as freeze_super() leading to
+	 * deadlocks and other problems.
 	 */
-	smp_wmb();
-	sb->s_flags |= SB_RDONLY;
 }
 
 static void update_super_work(struct work_struct *work)
-- 
2.35.3


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ