super: fixes for freezing, reconfiguration and error reporting

do_emergency_remount_callback() is called from __iterate_supers() with s_umount held exclusively. It creates a reconfigure context for the superblock and fs_context_for_reconfigure() takes an active reference for it. put_fs_context() drops that reference via deactivate_super() while s_umount is still held.

The iterator itself only holds a passive reference. So if the filesystem is unmounted while reconfigure_super() runs, the umount drops the mount's active reference without ever taking s_umount because atomic_add_unless() in deactivate_super() sees the context's reference. Now the context holds the last active reference and we're fscked:
put_fs_context() ends up in deactivate_super() which tries to take s_umount again and the kworker deadlocks on itself:

WARNING: possible recursive locking detected kworker/1:1/53 is trying to acquire lock:
(&type->s_umount_key#26){+.+.}-{4:4}, at: deactivate_super+0x6f/0x90 but task is already holding lock:
(&type->s_umount_key#26){+.+.}-{4:4}, at: super_lock+0x188/0x3a0
...

   down_write+0x90/0x1e0
   deactivate_super+0x6f/0x90
   put_fs_context+0xb1/0x7d0
   __iterate_supers+0x33d/0x410
   do_emergency_remount+0x20/0x40

"Emergency Remount complete" is never printed and the superblock's s_umount stays held forever. Everything that needs it hangs after that, including sync(2) and thus the clean reboot that usually follows a sysrq-u. The window is all of reconfigure_super(), i.e., syncing a read-write filesystem, so an umount racing with sysrq-u hits this easily.

Iterate without holding s_umount and take it in the callback instead. The context's active reference keeps the superblock alive while we hold s_umount and we only drop it after we released s_umount again. If it is the last reference deactivate_super() takes s_umount and shuts the filesystem down, just as the umount would have done.

Reproducer. sysrq-u remounts every block filesystem read-only, so use a throwaway VM. It needs CONFIG_SCSI_DEBUG and CONFIG_MAGIC_SYSRQ. The scsi_debug delay keeps reconfigure_super() busy writing back the dirty data long enough for the umount to land in the window:

modprobe scsi_debug dev_size_mb=256
dev=/dev/sda # the scsi_debug disk
mkfs.ext4 -q $dev
mount $dev /mnt
dd if=/dev/zero of=/mnt/dirty bs=1M count=32
echo 20 > /sys/bus/pseudo/drivers/scsi_debug/delay
echo u > /proc/sysrq-trigger
sleep 1
umount /mnt
# lockdep: possible recursive locking detected in
# do_emergency_remount(), "Emergency Remount complete" never shows up

  # and sync hangs.

Without the fix this reproduces on the first run on v7.3-rc6. With the fix the remount completes and the kworker shuts the filesystem down.

Fixes 8d0347f6c3a9 ("convert do_remount_sb() to fs_context")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/super.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

fs/super.c +19 -8

index 1d5ccf540a9b..15ecfbeb92ab 100644
@@ -1158,23 +1158,34 @@ int reconfigure_super(struct fs_context *fc)
 
 static void do_emergency_remount_callback(struct super_block *sb, void *unused)
 {
-	if (sb->s_bdev && !sb_rdonly(sb)) {
-		struct fs_context *fc;
+	struct fs_context *fc = NULL;
+
+	if (!super_lock_excl(sb))
+		return;
 
+	/* Make sure no reference put under s_umount is the last one. */
+	atomic_inc(&sb->s_active);
+	if (sb->s_bdev && !sb_rdonly(sb)) {
 		fc = fs_context_for_reconfigure(sb->s_root,
 					SB_RDONLY | SB_FORCE, SB_RDONLY);
-		if (!IS_ERR(fc)) {
-			if (parse_monolithic_mount_data(fc, NULL) == 0)
-				(void)reconfigure_super(fc);
-			put_fs_context(fc);
-		}
+		if (!IS_ERR(fc) && parse_monolithic_mount_data(fc, NULL) == 0)
+			(void)reconfigure_super(fc);
 	}
+	super_unlock_excl(sb);
+
+	/*
+	 * Our active reference may be the last one by now, so only drop it
+	 * after s_umount has been released.
+	 */
+	if (!IS_ERR_OR_NULL(fc))
+		put_fs_context(fc);
+	deactivate_super(sb);
 }
 
 static void do_emergency_remount(struct work_struct *work)
 {
 	__iterate_supers(do_emergency_remount_callback, NULL,
-			 SUPER_ITER_EXCL | SUPER_ITER_REVERSE);
+			 SUPER_ITER_UNLOCKED | SUPER_ITER_REVERSE);
 	kfree(work);
 	printk("Emergency Remount complete\n");
 }

Signature · 2 lines

-- 
2.53.0

filesystems_thaw() is called on every resume from suspend or hibernation no matter whether filesystem freezing was enabled. It walks all superblocks that have freeze methods and calls thaw_super() with FREEZE_EXCL | FREEZE_HOLDER_KERNEL and its owner token. It relies on may_unfreeze() to refuse every superblock it didn't freeze itself.

But may_unfreeze() checks the kernel freeze count before it checks whether the superblock is exclusively frozen at all. A superblock that userspace froze via FIFREEZE or that got frozen through its block device has freeze_kcount == 0. So every resume while such a filesystem is frozen triggers:

  WARNING: fs/super.c:2165 at thaw_super_locked+0x85f/0xbf0
  ...
   filesystems_thaw_callback+0x196/0x220

and with panic_on_warn the resume panics.

Check the owner first. If the superblock isn't exclusively frozen, or not by the caller, there's nothing for the caller to undo and that isn't a bug. An exclusive freeze always holds a kernel reference so keep warning if that's not the case.

Reproducer, needs CONFIG_PM_DEBUG:

  mount /dev/sda /mnt
  fsfreeze --freeze /mnt
  echo freezer > /sys/power/pm_test
  echo freeze > /sys/power/state
  # WARNING in thaw_super_locked() from filesystems_thaw_callback()

It fires on every run. Not a selftest because it suspends the machine and the only symptom is the warning in the kernel log.

Fixes 1af3331764b9 ("super: add filesystem freezing helpers for suspend and hibernate")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/super.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/super.c +3 -2

index 15ecfbeb92ab..4e8f7f48540c 100644
@@ -2165,14 +2165,15 @@ static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
 			return false;
 		if (WARN_ON_ONCE(!freeze_owner))
 			return false;
-		if (WARN_ON_ONCE(sb->s_writers.freeze_kcount == 0))
-			return false;
 		/* This isn't exclusively frozen. */
 		if (!sb->s_writers.freeze_owner)
 			return false;
 		/* This isn't exclusively frozen by us. */
 		if (sb->s_writers.freeze_owner != freeze_owner)
 			return false;
+		/* An exclusive freeze always holds a kernel reference. */
+		if (WARN_ON_ONCE(sb->s_writers.freeze_kcount == 0))
+			return false;
 		/*
 		 * This is still frozen multiple times so we're just
 		 * going to drop our reference count and undo our

Signature · 2 lines

-- 
2.53.0

quotactl_block() makes commands that modify quota wait for a frozen superblock to be thawed. Since 576215cffdef ("fs: Drop wait_unfrozen wait queue") it does that by taking and dropping s_writers via sb_start_write() and sb_end_write() and retrying until the superblock isn't frozen anymore.

But freeze_super() doesn't take s_writers for a read-only superblock. So sb_start_write() never waits and quotactl_block() spins until the superblock is thawed. The loop doesn't check for fatal signals and the permission checks only happen after quotactl_block() returned. So any user that can resolve the device node of a frozen read-only filesystem can make a task burn a cpu without being killable until someone thaws the filesystem.

Wait for the thaw with wait_var_event_killable() on s_writers.frozen instead. freeze_super() and thaw_super() already wake waiters on it for wait_for_partially_frozen(). That waiter only ever waits for the superblock to leave SB_FREEZE_WRITE and gets away without a barrier between the store to s_writers.frozen and wake_up_var() because percpu_down_write() executes a full barrier after the store that moves it out of SB_FREEZE_WRITE. That doesn't cover a waiter for SB_UNFROZEN. So add the barrier wake_up_var() requires to all of these wakeups.

The cond_resched() that 77449e453dfc ("quota: fix livelock between quotactl and freeze_super") added to the loop goes away with the spinning. As a side-effect waiting for a read-write superblock is killable now as well.

Fixes 576215cffdef ("fs: Drop wait_unfrozen wait queue")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/quota/quota.c | 12 ++++++++----
 fs/super.c       | 19 +++++++++++++------
 2 files changed, 21 insertions(+), 10 deletions(-)

fs/quota/quota.c +8 -4

index 33bacd707580..d55ef08de833 100644
@@ -894,11 +894,15 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
 			up_write(&sb->s_umount);
 		else
 			up_read(&sb->s_umount);
-		/* Wait for sb to unfreeze */
-		sb_start_write(sb);
-		sb_end_write(sb);
+		/*
+		 * Wait for sb to unfreeze. A read-only superblock is frozen
+		 * without taking s_writers so sb_start_write() won't wait.
+		 */
+		error = wait_var_event_killable(&sb->s_writers.frozen,
+				READ_ONCE(sb->s_writers.frozen) == SB_UNFROZEN);
 		put_super(sb);
-		cond_resched();
+		if (error)
+			return ERR_PTR(error);
 		goto retry;
 	}
 	return sb;

fs/super.c +13 -6

index 4e8f7f48540c..2dc4ec2e8014 100644
@@ -2068,6 +2068,13 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
 		percpu_up_write(sb->s_writers.rw_sem + level);
 }
 
+static inline void sb_freeze_wake(struct super_block *sb)
+{
+	/* wake_up_var() needs a full barrier after storing s_writers.frozen. */
+	smp_mb();
+	wake_up_var(&sb->s_writers.frozen);
+}
+
 static int wait_for_partially_frozen(struct super_block *sb)
 {
 	int ret = 0;

@@ -2293,7 +2300,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
 		WARN_ON_ONCE(freeze_inc(sb, who) > 1);
 		sb->s_writers.freeze_owner = freeze_owner;
 		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
-		wake_up_var(&sb->s_writers.frozen);
+		sb_freeze_wake(sb);
 		super_unlock_excl(sb);
 		return 0;
 	}

@@ -2313,7 +2320,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
 	if (ret) {
 		sb->s_writers.frozen = SB_UNFROZEN;
 		sb_freeze_unlock(sb, SB_FREEZE_PAGEFAULT);
-		wake_up_var(&sb->s_writers.frozen);
+		sb_freeze_wake(sb);
 		deactivate_locked_super(sb);
 		return ret;
 	}

@@ -2329,7 +2336,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
 				"VFS:Filesystem freeze failed\n");
 			sb->s_writers.frozen = SB_UNFROZEN;
 			sb_freeze_unlock(sb, SB_FREEZE_FS);
-			wake_up_var(&sb->s_writers.frozen);
+			sb_freeze_wake(sb);
 			deactivate_locked_super(sb);
 			return ret;
 		}

@@ -2341,7 +2348,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
 	WARN_ON_ONCE(freeze_inc(sb, who) > 1);
 	sb->s_writers.freeze_owner = freeze_owner;
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
-	wake_up_var(&sb->s_writers.frozen);
+	sb_freeze_wake(sb);
 	lockdep_sb_freeze_release(sb);
 	super_unlock_excl(sb);
 	return 0;

@@ -2378,7 +2385,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
 	if (sb_rdonly(sb)) {
 		sb->s_writers.frozen = SB_UNFROZEN;
 		sb->s_writers.freeze_owner = NULL;
-		wake_up_var(&sb->s_writers.frozen);
+		sb_freeze_wake(sb);
 		goto out_deactivate;
 	}
 

@@ -2396,7 +2403,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
 
 	sb->s_writers.frozen = SB_UNFROZEN;
 	sb->s_writers.freeze_owner = NULL;
-	wake_up_var(&sb->s_writers.frozen);
+	sb_freeze_wake(sb);
 	sb_freeze_unlock(sb, SB_FREEZE_FS);
 out_deactivate:
 	deactivate_locked_super(sb);

Signature · 2 lines

-- 
2.53.0

Add tests for quotactl() waiting on a frozen superblock:

- a waiter on a frozen read-only superblock can be killed
- a waiter on a frozen read-write superblock can be killed
- the waiters are woken up when the superblock is thawed

The tests need root, loop devices and mkfs.ext4.

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 tools/testing/selftests/Makefile                   |   1 +
 .../selftests/filesystems/freeze/.gitignore        |   2 +
 .../testing/selftests/filesystems/freeze/Makefile  |   7 +
 tools/testing/selftests/filesystems/freeze/config  |   3 +
 .../selftests/filesystems/freeze/loop_util.h       |  64 ++++++
 .../selftests/filesystems/freeze/quotactl_test.c   | 256 +++++++++++++++++++++
 6 files changed, 333 insertions(+)

tools/testing/selftests/Makefile +1 -0

index bc4fdb6f5337..a60778626c8c 100644
@@ -38,6 +38,7 @@ TARGETS += filesystems/epoll
 TARGETS += filesystems/eventfd
 TARGETS += filesystems/failfs
 TARGETS += filesystems/fat
+TARGETS += filesystems/freeze
 TARGETS += filesystems/openat2
 TARGETS += filesystems/open_tree_ns
 TARGETS += filesystems/overlayfs

tools/testing/selftests/filesystems/freeze/.gitignore new file +2 -0

new file mode 100644
index 000000000000..36fb6eaaf7eb
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+quotactl_test

tools/testing/selftests/filesystems/freeze/Makefile new file +7 -0

new file mode 100644
index 000000000000..f9e731412c56
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+CFLAGS += -Wall -O2 -g $(KHDR_INCLUDES)
+LOCAL_HDRS += loop_util.h
+
+TEST_GEN_PROGS := quotactl_test
+
+include ../../lib.mk

tools/testing/selftests/filesystems/freeze/config new file +3 -0

new file mode 100644
index 000000000000..5e0b39a87a05
@@ -0,0 +1,3 @@
+CONFIG_BLK_DEV_LOOP=y
+CONFIG_EXT4_FS=y
+CONFIG_QUOTA=y

tools/testing/selftests/filesystems/freeze/loop_util.h new file +64 -0

new file mode 100644
index 000000000000..fcea5be6030d
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SELFTESTS_FILESYSTEMS_FREEZE_LOOP_UTIL_H
+#define __SELFTESTS_FILESYSTEMS_FREEZE_LOOP_UTIL_H
+
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/loop.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+/* How often to retry when somebody else binds the free device first. */
+#define LOOP_BIND_TRIES 64
+
+/* Bind a free loop device to @fd; its number or -errno. */
+static inline int loop_bind(int fd)
+{
+	for (int i = 0; i < LOOP_BIND_TRIES; i++) {
+		char dev[32];
+		int cfd, lfd, n, err;
+
+		cfd = open("/dev/loop-control", O_RDWR | O_CLOEXEC);
+		if (cfd < 0)
+			return -errno;
+		n = ioctl(cfd, LOOP_CTL_GET_FREE);
+		err = errno;
+		close(cfd);
+		if (n < 0)
+			return -err;
+		snprintf(dev, sizeof(dev), "/dev/loop%d", n);
+		lfd = open(dev, O_RDWR | O_CLOEXEC);
+		if (lfd < 0)
+			return -errno;
+		err = ioctl(lfd, LOOP_SET_FD, fd) ? errno : 0;
+		close(lfd);
+		if (!err)
+			return n;
+		if (err != EBUSY)
+			return -err;
+	}
+	return -EBUSY;
+}
+
+/* Did loop_bind() fail because there are no loop device nodes or no driver? */
+static inline bool loop_missing(int ret)
+{
+	return ret == -ENOENT || ret == -ENODEV;
+}
+
+static inline void loop_clear(int n)
+{
+	char dev[32];
+	int fd;
+
+	snprintf(dev, sizeof(dev), "/dev/loop%d", n);
+	fd = open(dev, O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		return;
+	ioctl(fd, LOOP_CLR_FD, 0);
+	close(fd);
+}
+
+#endif /* __SELFTESTS_FILESYSTEMS_FREEZE_LOOP_UTIL_H */

tools/testing/selftests/filesystems/freeze/quotactl_test.c new file +256 -0

new file mode 100644
index 000000000000..24772e531eb4
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * quotactl(2) on a frozen superblock.
+ *
+ * Commands that modify quota wait in quotactl_block() for a frozen
+ * superblock to be thawed. A read-only superblock is frozen without
+ * taking s_writers so that wait can't be built on sb_start_write(). Check
+ * that a caller waiting on a frozen read-only or read-write superblock can
+ * be killed and that it is woken up when the superblock is thawed.
+ */
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/fs.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include <sys/quota.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <time.h>
+#include <unistd.h>
+
+#include "../../kselftest_harness.h"
+#include "loop_util.h"
+
+#define IMG_SIZE (64 << 20)
+#define POLL_MS 10
+/* How long the child gets to block in quotactl() or to go away. */
+#define WAIT_MS 3000
+
+FIXTURE(frozen) {
+	char dir[64];
+	char img[96];
+	char mnt[96];
+	char dev[32];
+	int loop;
+	int mntfd;
+	pid_t pid;
+	bool frozen;
+};
+
+FIXTURE_VARIANT(frozen) {
+	unsigned long flags;
+};
+
+FIXTURE_VARIANT_ADD(frozen, ro) {
+	.flags = MS_RDONLY,
+};
+
+FIXTURE_VARIANT_ADD(frozen, rw) {
+	.flags = 0,
+};
+
+/* Fork a child that calls quotactl(Q_SETQUOTA) on @dev and exits. */
+static pid_t quotactl_child(const char *dev)
+{
+	pid_t pid;
+
+	pid = fork();
+	if (pid == 0) {
+		struct if_dqblk dq = {};
+
+		quotactl(QCMD(Q_SETQUOTA, USRQUOTA), dev, 0, (void *)&dq);
+		_exit(0);
+	}
+	return pid;
+}
+
+/* Reap @pid within @ms milliseconds; true if it went away. */
+static bool reap(pid_t pid, int *status, int ms)
+{
+	struct timespec ts = { .tv_nsec = POLL_MS * 1000 * 1000 };
+
+	for (int i = 0; i < ms / POLL_MS; i++) {
+		pid_t ret = waitpid(pid, status, WNOHANG);
+
+		if (ret == pid)
+			return true;
+		if (ret < 0)
+			return false;
+		nanosleep(&ts, NULL);
+	}
+	return false;
+}
+
+/* Wait up to @ms milliseconds for @pid to sleep in the kernel. */
+static bool blocked(pid_t pid, int ms)
+{
+	struct timespec ts = { .tv_nsec = POLL_MS * 1000 * 1000 };
+	char path[64], buf[512], *p, state;
+
+	snprintf(path, sizeof(path), "/proc/%d/stat", pid);
+	for (int i = 0; i < ms / POLL_MS; i++) {
+		ssize_t n;
+		int fd;
+
+		fd = open(path, O_RDONLY | O_CLOEXEC);
+		if (fd < 0)
+			return false;
+		n = read(fd, buf, sizeof(buf) - 1);
+		close(fd);
+		if (n <= 0)
+			return false;
+		buf[n] = '\0';
+		/* "pid (comm) state ..." */
+		p = strrchr(buf, ')');
+		if (p && sscanf(p, ") %c", &state) == 1 &&
+		    (state == 'D' || state == 'S'))
+			return true;
+		nanosleep(&ts, NULL);
+	}
+	return false;
+}
+
+static void frozen_cleanup(FIXTURE_DATA(frozen) *self)
+{
+	int status;
+
+	if (self->pid > 0)
+		kill(self->pid, SIGKILL);
+	/* The thaw also lets a waiter go that ignored SIGKILL. */
+	if (self->frozen)
+		ioctl(self->mntfd, FITHAW, 0);
+	if (self->pid > 0)
+		reap(self->pid, &status, WAIT_MS);
+	if (self->mntfd >= 0) {
+		close(self->mntfd);
+		umount2(self->mnt, MNT_DETACH);
+	}
+	if (self->loop >= 0)
+		loop_clear(self->loop);
+	unlink(self->img);
+	rmdir(self->mnt);
+	rmdir(self->dir);
+}
+
+FIXTURE_SETUP(frozen)
+{
+	char cmd[160];
+	int fd, ret;
+
+	self->loop = -1;
+	self->mntfd = -1;
+	self->pid = -1;
+	self->frozen = false;
+
+	if (geteuid() != 0)
+		SKIP(return, "test requires CAP_SYS_ADMIN");
+	if (access("/dev/loop-control", R_OK | W_OK))
+		SKIP(return, "test requires loop devices");
+	/* Without a device this fails with ENODEV, or ENOSYS without quotactl(). */
+	if (quotactl(QCMD(Q_GETINFO, USRQUOTA), NULL, 0, NULL) && errno == ENOSYS)
+		SKIP(return, "kernel lacks quotactl()");
+
+	ASSERT_EQ(unshare(CLONE_NEWNS), 0);
+	ASSERT_EQ(mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL), 0);
+
+	snprintf(self->dir, sizeof(self->dir), "/tmp/freeze_quotactl.XXXXXX");
+	ASSERT_NE(mkdtemp(self->dir), NULL);
+	snprintf(self->img, sizeof(self->img), "%s/img", self->dir);
+	snprintf(self->mnt, sizeof(self->mnt), "%s/mnt", self->dir);
+	/* The harness doesn't run the teardown when the setup fails. */
+	ASSERT_EQ(mkdir(self->mnt, 0700), 0)
+		frozen_cleanup(self);
+
+	fd = open(self->img, O_RDWR | O_CREAT | O_CLOEXEC, 0600);
+	ASSERT_GE(fd, 0)
+		frozen_cleanup(self);
+	ASSERT_EQ(ftruncate(fd, IMG_SIZE), 0)
+		frozen_cleanup(self);
+
+	snprintf(cmd, sizeof(cmd), "mkfs.ext4 -q -F %s >/dev/null 2>&1", self->img);
+	ret = system(cmd);
+	if (ret) {
+		close(fd);
+		frozen_cleanup(self);
+		SKIP(return, "mkfs.ext4 failed or is not available");
+	}
+
+	self->loop = loop_bind(fd);
+	close(fd);
+	if (loop_missing(self->loop)) {
+		frozen_cleanup(self);
+		SKIP(return, "test requires loop devices");
+	}
+	ASSERT_GE(self->loop, 0)
+		frozen_cleanup(self);
+	snprintf(self->dev, sizeof(self->dev), "/dev/loop%d", self->loop);
+
+	if (mount(self->dev, self->mnt, "ext4", variant->flags, NULL)) {
+		if (errno == ENODEV) {
+			frozen_cleanup(self);
+			SKIP(return, "kernel lacks ext4");
+		}
+		ASSERT_EQ(errno, 0)
+			frozen_cleanup(self);
+	}
+	self->mntfd = open(self->mnt, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	ASSERT_GE(self->mntfd, 0)
+		frozen_cleanup(self);
+}
+
+FIXTURE_TEARDOWN(frozen)
+{
+	frozen_cleanup(self);
+}
+
+TEST_F(frozen, waiter_is_killable)
+{
+	int status;
+
+	ASSERT_EQ(ioctl(self->mntfd, FIFREEZE, 0), 0);
+	self->frozen = true;
+
+	self->pid = quotactl_child(self->dev);
+	ASSERT_GT(self->pid, 0);
+	ASSERT_TRUE(blocked(self->pid, WAIT_MS))
+		TH_LOG("quotactl() doesn't wait on a frozen superblock");
+
+	ASSERT_EQ(kill(self->pid, SIGKILL), 0);
+	ASSERT_TRUE(reap(self->pid, &status, WAIT_MS))
+		TH_LOG("quotactl() waiter survived SIGKILL on a frozen superblock");
+	self->pid = -1;
+	ASSERT_TRUE(WIFSIGNALED(status));
+	ASSERT_EQ(WTERMSIG(status), SIGKILL);
+}
+
+TEST_F(frozen, waiter_wakes_on_thaw)
+{
+	int status;
+
+	ASSERT_EQ(ioctl(self->mntfd, FIFREEZE, 0), 0);
+	self->frozen = true;
+
+	self->pid = quotactl_child(self->dev);
+	ASSERT_GT(self->pid, 0);
+	ASSERT_TRUE(blocked(self->pid, WAIT_MS))
+		TH_LOG("quotactl() doesn't wait on a frozen superblock");
+
+	ASSERT_EQ(ioctl(self->mntfd, FITHAW, 0), 0);
+	self->frozen = false;
+
+	ASSERT_TRUE(reap(self->pid, &status, WAIT_MS))
+		TH_LOG("quotactl() waiter wasn't woken by the thaw");
+	self->pid = -1;
+	ASSERT_TRUE(WIFEXITED(status));
+	ASSERT_EQ(WEXITSTATUS(status), 0);
+}
+
+TEST_HARNESS_MAIN

Signature · 2 lines

-- 
2.53.0

fserror_unmount() drops the bias on s_pending_errors and waits for the count to reach zero in wait_var_event() while the umount holds s_umount exclusively. The worker that finishes the last pending error wakes it up in fserror_pending_dec() through refcount_dec_and_test() and wake_up_var().

wake_up_var() requires a full barrier between the store that makes the condition true and its waitqueue_active() check. refcount_dec_and_test() isn't fully ordered though. It provides release ordering and acquire ordering on success which orders the load of the decrement but not its store against the later load of the waitqueue. So the worker can see an empty waitqueue while the umount, already queued, still reads a count of one. The umount then sleeps forever with s_umount held and sync(2) and everything else that wants that superblock hangs behind it.

LKMM allows this and so do the herd7 models for arm64 (LDADDL; DMB ISHLD; LDR) and POWER. x86 is safe because the locked xadd is a full barrier. With smp_mb() before wake_up_var() all three forbid it.

The litmus test (LKMM: Sometimes; with smp_mb() before the read of wq in
P1: Never). P0 is fserror_unmount() dropping the bias and queueing in wait_var_event(), P1 is the last fserror_pending_dec():

{ cnt=2; wq=0; state=0; }

  P0(atomic_t *cnt, int *wq, int *state, spinlock_t *wql)
  {
          int r0, r1, r2;

          r0 = atomic_fetch_sub_release(1, cnt);
          if (r0 == 2) {
                  r1 = atomic_read(cnt);
                  if (r1 == 1) {
                          spin_lock(wql);
                          WRITE_ONCE(*wq, 1);
                          smp_store_mb(*state, 2);
                          spin_unlock(wql);
                          r2 = atomic_read(cnt);
                  }
          }

}

  P1(atomic_t *cnt, int *wq, int *woken, spinlock_t *wql)
  {
          int r3, r4;

          r3 = atomic_fetch_sub_release(1, cnt);
          if (r3 == 1) {
                  smp_rmb();
                  r4 = READ_ONCE(*wq);
                  if (r4 != 0) {
                          spin_lock(wql);
                          WRITE_ONCE(*woken, 1);
                          spin_unlock(wql);
                  }
          }

}

exists (0:r0=2 /\ 0:r1=1 /\ 0:r2=1 /\ 1:r3=1 /\ 1:r4=0)

Not reproducible on x86 hardware. The consequence can be shown there by emulating the reordering in fserror_worker(), debug only: sample waitqueue_active() before the decrement, sleep, then decrement and only wake if the sample saw a waiter:

          iput(event->inode);
  +       if (debug_reorder_ms) {
  +               struct super_block *dsb = event->sb;
  +               bool waiter;
  +
  +               mempool_free(event, &fserror_events_pool);
  +               waiter = waitqueue_active(__var_waitqueue(&dsb->s_pending_errors));
  +               msleep(debug_reorder_ms);
  +               if (refcount_dec_and_test(&dsb->s_pending_errors) && waiter)
  +                       wake_up_var(&dsb->s_pending_errors);
  +               return;
  +       }
          fserror_free_event(event);

  # boot with fserror.debug_reorder_ms=5000
  mount -t ext4 /dev/vda /mnt
  xfs_io -x -c "shutdown" /mnt      # EXT4_IOC_SHUTDOWN with NOLOGFLUSH
  umount /mnt &                     # hangs forever
  sync &                            # hangs behind s_umount

  INFO: task umount:108 blocked for more than 30 seconds.
   fserror_unmount+0x20d/0x2c0
   generic_shutdown_super+0xac/0x530
   kill_block_super+0x3b/0xa0
   ext4_kill_sb+0x6c/0xc0
  INFO: task sync:111 <reader> blocked on an rw-semaphore likely owned by task umount:108 <writer>

Fixes 21945e6cb516 ("fs: report filesystem and file I/O errors to fsnotify")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/fserror.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

fs/fserror.c +8 -1

index 1e4d11fd9562..d8a800580012 100644
@@ -35,8 +35,15 @@ void fserror_unmount(struct super_block *sb)
 
 static inline void fserror_pending_dec(struct super_block *sb)
 {
-	if (refcount_dec_and_test(&sb->s_pending_errors))
+	if (refcount_dec_and_test(&sb->s_pending_errors)) {
+		/*
+		 * refcount_dec_and_test() isn't fully ordered but wake_up_var()
+		 * needs a full barrier between the store and its waitqueue check.
+		 * Pairs with the barrier in prepare_to_wait_event().
+		 */
+		smp_mb();
 		wake_up_var(&sb->s_pending_errors);
+	}
 }
 
 static inline void fserror_free_event(struct fserror_event *event)

Signature · 2 lines

-- 
2.53.0

alloc_super() biases s_pending_errors for every superblock but generic_shutdown_super() only drops the bias and waits for pending error reports if sb->s_root is set. fserror_alloc_event() accepts reports whenever SB_ACTIVE is set. But xfs sets SB_ACTIVE around log recovery and f2fs around gc for checkpoint=disable, both while filling the superblock. If such a mount fails s_root stays NULL, nothing waits for the queued reports and destroy_super_work() frees the superblock while fserror_worker() is still pending. The worker holds no reference to the superblock. It reads s_flags and s_op, puts the inode it pinned and decrements s_pending_errors:

  BUG: KASAN: slab-use-after-free in fserror_worker (fs/fserror.c:54)
  Read of size 8 at addr ff11000107366060 by task kworker/1:0/25
  ...
  Allocated by task 120:
   __kmalloc_cache_noprof+0x27a/0x650
   sget_fc (fs/super.c:326 [alloc_super()] fs/super.c:874)
   get_tree_bdev_flags (fs/super.c:1479 fs/super.c:1866)
   vfs_get_tree+0x8b/0x2f0
  Freed by task 83:
   kfree+0x1e8/0x5e0
   process_one_work+0x953/0x1700

Nobody can listen for errors of a superblock that isn't born yet. fanotify marks need a path in the mounted filesystem and the xfs health monitor is attached through an ioctl on a mounted filesystem as well. So don't accept reports before SB_BORN is set. A born superblock always has s_root set when it's shut down so every accepted report is waited for. Errors during the mount are still logged as lost reports and xfs still records them in its health state.

Waiting for the reports of a failed mount in generic_shutdown_super() instead wouldn't be enough. That runs from ->kill_sb() after the error path of ->fill_super() and xfs and f2fs have already freed state there that putting a pinned inode needs.

Reproducer, xfs with a dirty log and a bad crc in the refcount btree root of AG 0. Log recovery reports the corruption while xfs has SB_ACTIVE set and then the mount fails. The work item normally runs right away, so a kprobe on queue_work_on() points it at a wrapper that sleeps until destroy_super_work() has freed the superblock:

  mkfs.xfs -f /dev/vda
  mount /dev/vda /mnt && echo foo > /mnt/foo && sync
  xfs_io -x -c "shutdown -f" /mnt      # flush the log, then shut down
  umount /mnt                           # the log stays dirty
  xfs_db -x -c "agf 0" -c "addr refcntroot" -c "write -c numrecs 7" /dev/vda
  insmod r6_fshold.ko fserror_worker=0x$(awk '$3 == "fserror_worker" { print $1 }' /proc/kallsyms)
  echo 1 > /sys/module/r6_fshold/parameters/armed
  mount /dev/vda /mnt

XFS (vda): Metadata CRC error detected at xfs_refcountbt_read_verify+0x1e/0x240, xfs_refcountbt block 0x30 XFS (vda): Failed to recover leftover CoW staging extents, err -117.
XFS (vda): Error -5 reserving per-AG metadata reserve pool.

  BUG: KASAN: slab-use-after-free in fserror_worker+0x2da/0x310

Not a selftest since the work item has to be held back with a kprobe.

Fixes 21945e6cb516 ("fs: report filesystem and file I/O errors to fsnotify")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/fserror.c | 7 +++++++
 1 file changed, 7 insertions(+)

fs/fserror.c +7 -0

index d8a800580012..90ce04d58514 100644
@@ -96,6 +96,13 @@ static inline struct fserror_event *fserror_alloc_event(struct super_block *sb,
 		return NULL;
 	if (!(sb->s_flags & SB_ACTIVE))
 		goto out_pending;
+	/*
+	 * xfs and f2fs set SB_ACTIVE while filling the superblock. Nobody can
+	 * listen before the superblock is born and if the mount fails nothing
+	 * waits for the event.
+	 */
+	if (!(sb->s_flags & SB_BORN))
+		goto out_pending;
 
 	event = mempool_alloc(&fserror_events_pool, gfp_flags);
 	if (!event)

Signature · 2 lines

-- 
2.53.0

fs_bdev_freeze() freezes the superblocks on the device and then returns the error of sync_blockdev(). But bdev_freeze() takes an error from ->freeze() to mean that nothing is frozen and drops bd_fsfreeze_count again. Its callers act accordingly. dm's lock_fs() doesn't set DMF_FROZEN and the shutdown ioctls just return. So nobody ever calls bdev_thaw() and the superblock stays frozen until someone thaws it by hand. Until then every writer blocks in sb_start_write().

freeze_super() only syncs sb->s_bdev and fails cleanly if that reports an error. So the sync in fs_bdev_freeze() is the first to see a writeback error on any other device of the filesystem, an external log or journal device or a realtime device. It also is the first sync for a read-only superblock since freeze_super() doesn't sync those at all.

I made fs_bdev_freeze() return that error in fbcb8f39e96d ("bdev:
surface the error from sync_blockdev()") without thinking about what it means for the superblocks that are already frozen at that point. Don't report a failed sync once a superblock was frozen. The device is frozen and the regular bdev_thaw() undoes it. The error isn't lost either. It stays recorded in the errseq of the mapping so fsync() on the device still reports it. A device without any live superblock still reports the error of the sync, just like bdev_freeze() does for a device without holder ops.

Reproducer, xfs with an external log on dm-linear, a stale writeback error on the log device, then a dm suspend:

  mkfs.xfs -f -l logdev=/dev/vdb,size=64m /dev/vda     # vdb is 128M
  dmsetup create log --table "0 $(blockdev --getsz /dev/vdb) linear /dev/vdb 0"
  mount -o logdev=/dev/mapper/log /dev/vda /mnt
  echo 1 > /sys/kernel/debug/fail_make_request/times
  echo 100 > /sys/kernel/debug/fail_make_request/probability
  echo 1 > /sys/block/dm-0/make-it-fail
  xfs_io -c "pwrite 100m 4k" -c "sync_range -w 100m 4k" /dev/mapper/log
  echo 0 > /sys/block/dm-0/make-it-fail
  dmsetup suspend log          # fails with -EIO, the device stays active
  touch /mnt/foo               # hangs until fsfreeze --unfreeze /mnt

Buffer I/O error on dev dm-0, logical block 25600, lost async page write
device-mapper: suspend ioctl on log failed: Input/output error

  INFO: task touch:117 blocked for more than 30 seconds.
   percpu_rwsem_wait+0x203/0x500
   mnt_want_write+0x28f/0x2f0
   lookup_open+0x135/0x1b20

The selftest in the next patch covers the read-only case.

Fixes fbcb8f39e96d ("bdev: surface the error from sync_blockdev()")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/super.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/super.c +13 -3

index 2dc4ec2e8014..82290d094c03 100644
@@ -1555,7 +1555,8 @@ static void fs_bdev_sync(struct block_device *bdev)
  *
  * Return: 0, or the error from the one superblock on a single-fs device.  When
  *         several superblocks share @bdev a per-superblock failure is swallowed
- *         (see below), but a sync_blockdev() failure is always reported.
+ *         (see below).  A sync_blockdev() failure is only reported if no
+ *         live superblock uses @bdev.
  */
 static int fs_bdev_freeze(struct block_device *bdev)
 {

@@ -1586,8 +1587,17 @@ static int fs_bdev_freeze(struct block_device *bdev)
 	 */
 	if (error && count > 1)
 		error = 0;
-	if (!error)
-		error = sync_blockdev(bdev);
+	if (error)
+		return error;
+
+	/*
+	 * An error tells bdev_freeze() that nothing is frozen and nobody would
+	 * ever thaw a superblock frozen above. So only report a failed sync if
+	 * no superblock uses the device.
+	 */
+	error = sync_blockdev(bdev);
+	if (count)
+		error = 0;
 	return error;
 }
 

Signature · 2 lines

-- 
2.53.0

Device-mapper and the filesystem shutdown ioctls freeze the filesystems on a device through bdev_freeze() and fs_bdev_freeze() freezes each superblock with FREEZE_HOLDER_USERSPACE | FREEZE_MAY_NEST. That's the same holder FIFREEZE uses. So FITHAW can drop that freeze while the device is still suspended. qemu-guest-agent's guest-fsfreeze-thaw does just that as it issues FITHAW on every mount until it fails. An emergency thaw does the same. do_thaw_all_callback() only thaws the device freezes of sb->s_bdev and then drops the remaining userspace freeze of the superblock which may belong to any other device of the filesystem, an external log or journal device or another btrfs member.

The device's bdev_thaw() then calls fs_bdev_thaw() which gets -EINVAL from thaw_super() since there's nothing left to thaw. bdev_thaw() takes that to mean that the device is still frozen and keeps bd_fsfreeze_count elevated and device-mapper ignores the error. So the device stays frozen for good and every later mount of it fails:

dm-0: Can't mount, blockdev is frozen

fe967191e585 ("fs: don't return -EINVAL for successful nested thaw") fixed the same outcome for a freeze that nested with FIFREEZE. A freeze that lost its hold behind the device's back still ends up there.

Treat -EINVAL from thaw_super() in fs_bdev_thaw() as the freeze being gone already. There's nothing left to thaw for the device then. Before fs_bdev_thaw() existed thaw_bdev() kept the count on the same error.

Fixes 49ef8832fb1a ("bdev: implement freeze and thaw holder operations")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/super.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

fs/super.c +24 -4

index 82290d094c03..ac4c4d067509 100644
@@ -1494,10 +1494,29 @@ static int fs_super_freeze(struct super_block *sb)
 
 static int fs_super_thaw(struct super_block *sb)
 {
-	if (sb->s_op->thaw_super)
-		return sb->s_op->thaw_super(sb,
+	int error;
+
+	if (sb->s_op->thaw_super) {
+		error = sb->s_op->thaw_super(sb,
 				FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
-	return thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
+		/* -EINVAL means the freeze is gone already. */
+		if (error == -EINVAL)
+			return 0;
+		return error;
+	}
+
+	/* The caller holds an active reference. */
+	if (WARN_ON_ONCE(!super_lock_excl(sb)))
+		return 0;
+
+	/* FITHAW or an emergency thaw may have dropped the freeze already. */
+	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE ||
+	    !sb->s_writers.freeze_ucount) {
+		super_unlock_excl(sb);
+		return 0;
+	}
+	return thaw_super_locked(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE,
+				 NULL);
 }
 
 static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)

@@ -1610,7 +1629,8 @@ static int fs_bdev_freeze(struct block_device *bdev)
  * frozen more than once (by the kernel or via another device).
  *
  * Return: 0, or the first error on a single-fs device; a shared device swallows
- *         per-superblock errors, as fs_bdev_freeze() does.
+ *         per-superblock errors, as fs_bdev_freeze() does. A superblock that
+ *         lost the freeze already, e.g., to FITHAW, counts as thawed.
  */
 static int fs_bdev_thaw(struct block_device *bdev)
 {

Signature · 2 lines

-- 
2.53.0

I'd just note that interpretting EINVAL from ->thaw_super is a bit of an adventure but so far only gfs2 implements that and that looks safe. Using some less abused error code to mean "already thawed" would be probably more robust in the long run...

Quoted 4 lines · Jan Kara

I'd just note that interpretting EINVAL from ->thaw_super is a bit of an adventure but so far only gfs2 implements that and that looks safe. Using some less abused error code to mean "already thawed" would be probably more robust in the long run...

Something like the below?

From: Christian Brauner <[email protected]>
Date: Tue, 10 Nov 2026 16:39:31 +0100
Subject: [PATCH] super: return -EALREADY for a thaw that finds nothing to drop

thaw_super_locked() returns -EINVAL when the superblock isn't frozen and also when it is frozen but not by the holder that asks to thaw it. gfs2's ->thaw_super() returns -EINVAL as well when this node never initiated the freeze. So when fs_super_thaw() thaws the superblocks of a block device whose freeze is gone already -EINVAL from ->thaw_super() is all it has to go on and it has to guess that this is what happened.

Use -EALREADY for a thaw that has nothing to drop. Nothing in the freeze and thaw code returns it so far and it clearly communicates what happened. A freeze that is held by someone else keeps returning -EINVAL.

FITHAW has always reported an unfrozen filesystem as -EINVAL and fsfreeze and qemu-guest-agent rely on that, so ioctl_fsthaw() translates. The other in-kernel callers thaw their own kernel freezes and pass the error on without looking at it.

Suggested-by Jan Kara <[email protected]>

Link https://lore.kernel.org/r/yl2umpbk6fnz2soh7fi5xellmovarx6ouxrvf4ki5xxledmihx@gjldiqbxpufn open here

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 Documentation/filesystems/vfs.rst |  4 +++-
 fs/gfs2/super.c                   |  2 +-
 fs/ioctl.c                        | 10 ++++++++--
 fs/super.c                        | 11 ++++++++---
 4 files changed, 20 insertions(+), 7 deletions(-)

Documentation/filesystems/vfs.rst +3 -1

index 1824462a43711..e6d332fa17be9 100644
@@ -294,7 +294,9 @@ or bottom half).
 
 ``thaw_super``
 	called when VFS is unlocking a filesystem and making it writable
-	again after ->freeze_super. Optional.
+	again after ->freeze_super. Optional. Return -EALREADY if there is
+	no freeze to drop. The VFS relies on that when it thaws the
+	filesystems of a block device.
 
 ``unfreeze_fs``
 	called when VFS is unlocking a filesystem and making it writable

fs/gfs2/super.c +1 -1

index 04bb4cf787d4d..ce0eb4d1a7031 100644
@@ -806,7 +806,7 @@ static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who,
 		return -EBUSY;
 	if (!test_bit(SDF_FREEZE_INITIATOR, &sdp->sd_flags)) {
 		mutex_unlock(&sdp->sd_freeze_mutex);
-		return -EINVAL;
+		return -EALREADY;
 	}
 
 	atomic_inc(&sb->s_active);

fs/ioctl.c +8 -2

index 1c152c2b1b67d..56acaa122ec4d 100644
@@ -402,14 +402,20 @@ static int ioctl_fsfreeze(struct file *filp)
 static int ioctl_fsthaw(struct file *filp)
 {
 	struct super_block *sb = file_inode(filp)->i_sb;
+	int error;
 
 	if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN))
 		return -EPERM;
 
 	/* Thaw */
 	if (sb->s_op->thaw_super)
-		return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
-	return thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
+		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
+	else
+		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
+	/* FITHAW has always reported an unfrozen filesystem as -EINVAL. */
+	if (error == -EALREADY)
+		error = -EINVAL;
+	return error;
 }
 
 static int ioctl_file_dedupe_range(struct file *file,

fs/super.c +8 -3

index cd6f57ababe08..72ffa5527dda2 100644
@@ -1640,8 +1640,8 @@ static int fs_super_thaw(struct super_block *sb)
 	if (sb->s_op->thaw_super) {
 		error = sb->s_op->thaw_super(sb,
 				FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
-		/* -EINVAL means the freeze is gone already. */
-		if (error == -EINVAL)
+		/* The freeze is gone already. */
+		if (error == -EALREADY)
 			return 0;
 		return error;
 	}

@@ -2587,8 +2587,11 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
 {
 	int error = -EINVAL;
 
-	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
+	/* There's no freeze to drop. */
+	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
+		error = -EALREADY;
 		goto out_unlock;
+	}
 
 	if (!may_unfreeze(sb, who, freeze_owner))
 		goto out_unlock;

@@ -2652,6 +2655,8 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
  * A filesystem may hold multiple devices and thus a filesystems may
  * have been frozen through the block layer via multiple block devices.
  * The filesystem remains frozen until all block devices are unfrozen.
+ *
+ * Returns -EALREADY if the filesystem isn't frozen.
  */
 int thaw_super(struct super_block *sb, enum freeze_holder who,
 	       const void *freeze_owner)

Signature · 4 lines

2.53.0


-- 

Add a test for a filesystem that is thawed while device-mapper holds a freeze on it:

- mount ext4 read-only on a dm-linear device
- suspend the device with lockfs
- FITHAW the filesystem
- resume the device
- the device can be mounted again after the umount

The test needs root, loop devices, device-mapper and mkfs.ext4.

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 .../selftests/filesystems/freeze/.gitignore        |   1 +
 .../testing/selftests/filesystems/freeze/Makefile  |   2 +-
 .../selftests/filesystems/freeze/bdev_thaw_test.c  | 251 +++++++++++++++++++++
 tools/testing/selftests/filesystems/freeze/config  |   2 +
 4 files changed, 255 insertions(+), 1 deletion(-)

tools/testing/selftests/filesystems/freeze/.gitignore +1 -0

index bc6fc0fbb2a0..28f1fe73f19b 100644
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 quotactl_test
 bdev_freeze_test
+bdev_thaw_test

tools/testing/selftests/filesystems/freeze/Makefile +1 -1

index 53c8a373223f..acd246e1b14b 100644
@@ -3,6 +3,6 @@ CFLAGS += -Wall -O2 -g $(KHDR_INCLUDES)
 LOCAL_HDRS += loop_util.h
 
 TEST_GEN_PROGS := quotactl_test
-TEST_GEN_PROGS += bdev_freeze_test
+TEST_GEN_PROGS += bdev_freeze_test bdev_thaw_test
 
 include ../../lib.mk

tools/testing/selftests/filesystems/freeze/bdev_thaw_test.c new file +251 -0

new file mode 100644
index 000000000000..9bb21293093c
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Thawing a superblock behind the block layer's back.
+ *
+ * A device-mapper suspend freezes the filesystem on the device through
+ * bdev_freeze() with the same userspace holder FIFREEZE uses, so FITHAW can
+ * drop that freeze while the device is still suspended. The resume then
+ * finds nothing left to thaw. Check that it still thaws the device so that
+ * it can be mounted again afterwards. The filesystem is mounted read-only
+ * so the freeze and the thaw don't need to write to the suspended device.
+ */
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/dm-ioctl.h>
+#include <linux/fs.h>
+#include <sched.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <sys/sysmacros.h>
+#include <unistd.h>
+
+#include "../../kselftest_harness.h"
+#include "loop_util.h"
+
+#define IMG_SIZE (64 << 20)
+
+FIXTURE(bdev_thaw) {
+	char dir[64];
+	char img[96];
+	char mnt[96];
+	char node[96];
+	char name[32];
+	int loop;
+	int mntfd;
+	bool tmpfs;
+	bool dm;
+	bool suspended;
+	bool mounted;
+};
+
+static int dm_ioctl(unsigned long cmd, const char *name, uint32_t flags, dev_t *dev)
+{
+	struct dm_ioctl dmi = {
+		.version = { DM_VERSION_MAJOR, 0, 0 },
+		.data_size = sizeof(dmi),
+		.data_start = sizeof(dmi),
+		.flags = flags,
+	};
+	int fd, ret = 0;
+
+	strncpy(dmi.name, name, sizeof(dmi.name) - 1);
+	fd = open("/dev/mapper/control", O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		return -errno;
+	if (ioctl(fd, cmd, &dmi))
+		ret = -errno;
+	else if (dev)
+		*dev = makedev((dmi.dev >> 8) & 0xfff,
+			       (dmi.dev & 0xff) | ((dmi.dev >> 12) & 0xfff00));
+	close(fd);
+	return ret;
+}
+
+/* Remove @name now, or on its last close if udev still has it open. */
+static int dm_remove(const char *name)
+{
+	return dm_ioctl(DM_DEV_REMOVE, name, DM_DEFERRED_REMOVE, NULL);
+}
+
+/* Create @name as a linear mapping of all of @backing and make it live. */
+static int dm_create_linear(const char *name, dev_t backing, uint64_t sectors,
+			    dev_t *dev)
+{
+	struct {
+		struct dm_ioctl dmi;
+		struct dm_target_spec spec;
+		char params[64];
+	} __attribute__((aligned(8))) t = {};
+	int fd, ret;
+
+	ret = dm_ioctl(DM_DEV_CREATE, name, 0, NULL);
+	if (ret)
+		return ret;
+
+	t.dmi.version[0] = DM_VERSION_MAJOR;
+	t.dmi.data_size = sizeof(t);
+	t.dmi.data_start = sizeof(t.dmi);
+	t.dmi.target_count = 1;
+	strncpy(t.dmi.name, name, sizeof(t.dmi.name) - 1);
+	t.spec.length = sectors;
+	strcpy(t.spec.target_type, "linear");
+	snprintf(t.params, sizeof(t.params), "%u:%u 0", major(backing), minor(backing));
+
+	fd = open("/dev/mapper/control", O_RDWR | O_CLOEXEC);
+	if (fd >= 0) {
+		ret = ioctl(fd, DM_TABLE_LOAD, &t) ? -errno : 0;
+		close(fd);
+	} else {
+		ret = -errno;
+	}
+	/* Resuming makes the loaded table live. */
+	if (!ret)
+		ret = dm_ioctl(DM_DEV_SUSPEND, name, 0, dev);
+	if (ret)
+		dm_remove(name);
+	return ret;
+}
+
+static void bdev_thaw_cleanup(FIXTURE_DATA(bdev_thaw) *self)
+{
+	if (self->suspended)
+		dm_ioctl(DM_DEV_SUSPEND, self->name, 0, NULL);
+	if (self->mntfd >= 0)
+		close(self->mntfd);
+	if (self->mounted)
+		umount2(self->mnt, MNT_DETACH);
+	if (self->dm)
+		dm_remove(self->name);
+	if (self->loop >= 0)
+		loop_clear(self->loop);
+	unlink(self->node);
+	unlink(self->img);
+	rmdir(self->mnt);
+	if (self->tmpfs)
+		umount2(self->dir, MNT_DETACH);
+	rmdir(self->dir);
+}
+
+FIXTURE_SETUP(bdev_thaw)
+{
+	char cmd[160], dev[32];
+	struct stat st;
+	dev_t dmdev;
+	int fd, ret;
+
+	self->loop = -1;
+	self->mntfd = -1;
+	self->tmpfs = false;
+	self->dm = false;
+	self->suspended = false;
+	self->mounted = false;
+
+	if (geteuid() != 0)
+		SKIP(return, "test requires CAP_SYS_ADMIN");
+	if (access("/dev/loop-control", R_OK | W_OK))
+		SKIP(return, "test requires loop devices");
+	if (access("/dev/mapper/control", R_OK | W_OK))
+		SKIP(return, "test requires device-mapper");
+
+	ASSERT_EQ(unshare(CLONE_NEWNS), 0);
+	ASSERT_EQ(mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL), 0);
+
+	snprintf(self->dir, sizeof(self->dir), "/tmp/bdev_thaw.XXXXXX");
+	ASSERT_NE(mkdtemp(self->dir), NULL);
+	/* /tmp may be mounted nodev, the dm node needs a tmpfs of our own. */
+	ASSERT_EQ(mount("tmpfs", self->dir, "tmpfs", MS_NOSUID | MS_NOEXEC, NULL), 0)
+		rmdir(self->dir);
+	self->tmpfs = true;
+	snprintf(self->img, sizeof(self->img), "%s/img", self->dir);
+	snprintf(self->mnt, sizeof(self->mnt), "%s/mnt", self->dir);
+	snprintf(self->node, sizeof(self->node), "%s/dm", self->dir);
+	snprintf(self->name, sizeof(self->name), "bdev_thaw_%d", getpid());
+	/* The harness doesn't run the teardown when the setup fails. */
+	ASSERT_EQ(mkdir(self->mnt, 0700), 0)
+		bdev_thaw_cleanup(self);
+
+	fd = open(self->img, O_RDWR | O_CREAT | O_CLOEXEC, 0600);
+	ASSERT_GE(fd, 0)
+		bdev_thaw_cleanup(self);
+	ASSERT_EQ(ftruncate(fd, IMG_SIZE), 0)
+		bdev_thaw_cleanup(self);
+
+	snprintf(cmd, sizeof(cmd), "mkfs.ext4 -q -F %s >/dev/null 2>&1", self->img);
+	ret = system(cmd);
+	if (ret) {
+		close(fd);
+		bdev_thaw_cleanup(self);
+		SKIP(return, "mkfs.ext4 failed or is not available");
+	}
+
+	self->loop = loop_bind(fd);
+	close(fd);
+	if (loop_missing(self->loop)) {
+		bdev_thaw_cleanup(self);
+		SKIP(return, "test requires loop devices");
+	}
+	ASSERT_GE(self->loop, 0)
+		bdev_thaw_cleanup(self);
+	snprintf(dev, sizeof(dev), "/dev/loop%d", self->loop);
+	ASSERT_EQ(stat(dev, &st), 0)
+		bdev_thaw_cleanup(self);
+
+	ASSERT_EQ(dm_create_linear(self->name, st.st_rdev, IMG_SIZE >> 9, &dmdev), 0)
+		bdev_thaw_cleanup(self);
+	self->dm = true;
+	ASSERT_EQ(mknod(self->node, S_IFBLK | 0600, dmdev), 0)
+		bdev_thaw_cleanup(self);
+
+	if (mount(self->node, self->mnt, "ext4", MS_RDONLY, NULL)) {
+		if (errno == ENODEV) {
+			bdev_thaw_cleanup(self);
+			SKIP(return, "kernel lacks ext4");
+		}
+		ASSERT_EQ(errno, 0)
+			bdev_thaw_cleanup(self);
+	}
+	self->mounted = true;
+	self->mntfd = open(self->mnt, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	ASSERT_GE(self->mntfd, 0)
+		bdev_thaw_cleanup(self);
+}
+
+FIXTURE_TEARDOWN(bdev_thaw)
+{
+	bdev_thaw_cleanup(self);
+}
+
+TEST_F(bdev_thaw, fithaw_during_dm_suspend)
+{
+	/* A suspend with lockfs freezes the filesystem through bdev_freeze(). */
+	ASSERT_EQ(dm_ioctl(DM_DEV_SUSPEND, self->name, DM_SUSPEND_FLAG, NULL), 0);
+	self->suspended = true;
+
+	/* FITHAW drops the freeze the suspend holds. */
+	if (ioctl(self->mntfd, FITHAW, 0)) {
+		ASSERT_EQ(errno, EINVAL);
+		SKIP(return, "FITHAW doesn't drop a device-mapper freeze");
+	}
+
+	/* The resume's bdev_thaw() finds nothing left to thaw. */
+	ASSERT_EQ(dm_ioctl(DM_DEV_SUSPEND, self->name, 0, NULL), 0);
+	self->suspended = false;
+
+	close(self->mntfd);
+	self->mntfd = -1;
+	ASSERT_EQ(umount2(self->mnt, 0), 0);
+	self->mounted = false;
+
+	ASSERT_EQ(mount(self->node, self->mnt, "ext4", MS_RDONLY, NULL), 0)
+		TH_LOG("device still frozen after the resume: %s", strerror(errno));
+	self->mounted = true;
+}
+
+TEST_HARNESS_MAIN

tools/testing/selftests/filesystems/freeze/config +2 -0

index 09d86d9bc7d7..d03720cda9a9 100644
@@ -1,7 +1,9 @@
+CONFIG_BLK_DEV_DM=y
 CONFIG_BLK_DEV_LOOP=y
 CONFIG_DEBUG_FS=y
 CONFIG_EXT4_FS=y
 CONFIG_FAIL_MAKE_REQUEST=y
 CONFIG_FAULT_INJECTION=y
 CONFIG_FAULT_INJECTION_DEBUG_FS=y
+CONFIG_MD=y
 CONFIG_QUOTA=y

Signature · 2 lines

-- 
2.53.0

fspick() creates a reconfiguration context with an empty flag mask. Only setting "ro" or "rw" adds SB_RDONLY to it. reconfigure_super() honours the mask and leaves SB_RDONLY alone otherwise. But a lot of filesystems decide in their ->reconfigure() whether to go read-write by comparing fc->sb_flags & SB_RDONLY with the current state of the superblock and ignore the mask: affs, befs, btrfs, efivarfs, ext4, fat, gfs2, hfs, hfsplus, hpfs, isofs, jffs2, jfs, minix, nilfs2, ntfs, ntfs3, ocfs2, overlayfs, ubifs, udf, ufs and xfs. mount(2) always puts SB_RDONLY into the mask so that never mattered for it.

So reconfiguring a read-only superblock through fspick() without "rw", e.g., to change an unrelated option or with no option at all, makes these filesystems go through their read-only to read-write transition. For ext4 and btrfs that ends with a read-write superblock that writes to the device and can be written to through any read-write bind mount:

EXT4-fs (vda): re-mounted 68db7463-95cb-47f9-ab87-d1fd35252958 r/w.

Fill in the current read-only state when the mask doesn't cover it. reconfigure_super() is called with s_umount held so that can't race with another reconfiguration. Doing it when the context is created would.

Fixes cf3cba4a429b ("vfs: syscall: Add fspick() to select a superblock for reconfiguration")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/super.c | 8 ++++++++
 1 file changed, 8 insertions(+)

fs/super.c +8 -0

index ac4c4d067509..edd5648d1720 100644
@@ -1077,6 +1077,14 @@ int reconfigure_super(struct fs_context *fc)
 	if (sb->s_writers.frozen != SB_UNFROZEN)
 		return -EBUSY;
 
+	/*
+	 * Filesystems take fc->sb_flags & SB_RDONLY as the read-only state the
+	 * superblock ends up in. If that isn't being changed, e.g., fspick()
+	 * without "ro" or "rw", it's the current one.
+	 */
+	if (!(fc->sb_flags_mask & SB_RDONLY))
+		fc->sb_flags = (fc->sb_flags & ~SB_RDONLY) | (sb->s_flags & SB_RDONLY);
+
 	retval = security_sb_remount(sb, fc->security);
 	if (retval)
 		return retval;

Signature · 2 lines

-- 
2.53.0

Now that do_emergency_remount() takes s_umount itself in its callback nothing iterates with SUPER_ITER_EXCL anymore. Remove it and take s_umount shared in __iterate_supers().

No functional changes.

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/super.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

fs/super.c +4 -6

index edd5648d1720..657958a1d135 100644
@@ -935,9 +935,8 @@ void drop_super_exclusive(struct super_block *sb)
 }
 
 enum super_iter_flags_t {
-	SUPER_ITER_EXCL		= (1U << 0),
-	SUPER_ITER_UNLOCKED	= (1U << 1),
-	SUPER_ITER_REVERSE	= (1U << 2),
+	SUPER_ITER_UNLOCKED	= (1U << 0),
+	SUPER_ITER_REVERSE	= (1U << 1),
 };
 
 static inline struct super_block *first_super(enum super_iter_flags_t flags)

@@ -959,7 +958,6 @@ static void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
 			     enum super_iter_flags_t flags)
 {
 	struct super_block *sb, *p = NULL;
-	bool excl = flags & SUPER_ITER_EXCL;
 
 	spin_lock(&sb_lock);
 

@@ -976,9 +974,9 @@ static void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
 
 		if (flags & SUPER_ITER_UNLOCKED) {
 			f(sb, arg);
-		} else if (super_lock(sb, excl)) {
+		} else if (super_lock_shared(sb)) {
 			f(sb, arg);
-			super_unlock(sb, excl);
+			super_unlock_shared(sb);
 		}
 
 		if (p)

Signature · 2 lines

-- 
2.53.0

vfs_clean_context() prepares a context for the next reconfiguration after fsmount() and after every successful FSCONFIG_CMD_RECONFIGURE. It resets fc->sb_flags but leaves fc->sb_flags_mask alone. So the next FSCONFIG_CMD_RECONFIGURE on that file descriptor applies the old mask to empty flags.

That makes a read-only superblock read-write without anyone asking for it:

  fsfd = fsopen("ext4", 0);
  fsconfig(fsfd, FSCONFIG_SET_STRING, "source", "/dev/vda", 0);
  fsconfig(fsfd, FSCONFIG_SET_FLAG, "ro", NULL, 0);
  fsconfig(fsfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
  mntfd = fsmount(fsfd, 0, 0);
  move_mount(mntfd, "", AT_FDCWD, "/mnt", MOVE_MOUNT_F_EMPTY_PATH);
  fsconfig(fsfd, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0);

EXT4-fs (vda): re-mounted 68db7463-95cb-47f9-ab87-d1fd35252958 r/w.

The same happens for a second FSCONFIG_CMD_RECONFIGURE on a context from fspick() after the first one set "ro". The stale mask also drops SB_SYNCHRONOUS, SB_LAZYTIME and SB_MANDLOCK set in an earlier round. And btrfs_reconfigure() adds SB_POSIXACL to the mask when "acl" or "noacl" changes the ACL state so every later reconfiguration on that file descriptor fails with -EINVAL as reconfigure_super() refuses anything outside of MS_RMT_MASK.

Reset the mask as well so a cleaned context looks like a new one from fspick() just as the comment in sys_fsmount() promises.

Fixes ecdab150fddb ("vfs: syscall: Add fsconfig() for configuring and managing a context")

Cc [email protected]

Signed-off-by Christian Brauner (Amutable) <[email protected]>

---
 fs/fs_context.c | 1 +
 1 file changed, 1 insertion(+)

fs/fs_context.c +1 -0

index a50584df97be..ba99e673458a 100644
@@ -544,6 +544,7 @@ void vfs_clean_context(struct fs_context *fc)
 	fc->fs_private = NULL;
 	fc->s_fs_info = NULL;
 	fc->sb_flags = 0;
+	fc->sb_flags_mask = 0;
 	security_free_mnt_opts(&fc->security);
 	kfree(fc->source);
 	fc->source = NULL;

Signature · 2 lines

-- 
2.53.0

super: fixes for freezing, reconfiguration and error reporting

A pile of fixes for super.c and friends that I found while going over the freeze and reconfigure paths again. Most of them are old and all of them are tagged for stable. Most come with a selftest under tools/testing/selftests/filesystems/freeze/ that fails without the fix.

[...]