linux-kernel - [RFC][PATCH] fs: Prevent syncing frozen file system

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: <1436463945-12556-1-git-send-email-lczerner@redhat.com>
Date:	Thu,  9 Jul 2015 19:45:45 +0200
From:	Lukas Czerner <lczerner@...hat.com>
To:	viro@...IV.linux.org.uk
Cc:	bfields@...ldses.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, Lukas Czerner <lczerner@...hat.com>
Subject: [RFC][PATCH] fs: Prevent syncing frozen file system

Currently we can end up in a deadlock because of broken
sb_start_write -> s_umount ordering.

The race goes like this:

 - write the file
 - unlink the file - final_iput will not be calles as file is opened
 - freeze the file system
 - Now simultaneously close the file and call sync (or syncfs on that
   particular file system). Sync will get to wait_sb_inodes() where it will
   grab the referece to the inode (__iget()) and later to call iput().
   If we manage to close the file and drop the reference in between those
   calls sync will attempt to do a iput_final() because the inode is now
   unlinked and we're holding the last reference to it. This will
   however block on a frozen file system (ext4_delete_inode for
   example).

Note that I've not been able to reproduce the issue, I've only seen this
happen once. However with some instrumentation (like msleep() in the
wait_sb_inodes() it can be achieved.

Fix this by properly doing sb_start_write/sb_end_write to prevent us
from fsfreeze.

Note that with this patch syncfs will block on the frozen file system
which is probably ok, but sync will block if any file system happens to
be frozen - not sure if that's a problem, but it's certainly different
from what we've been used to.

Signed-off-by: Lukas Czerner <lczerner@...hat.com>
---
 fs/super.c | 7 +++++++
 fs/sync.c  | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index b613723..d337c91 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -514,10 +514,17 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
+		/*
+		 * Whatever we're going to do to the file system we have to
+		 * make sure that we'll not end up blocking on frozen file
+		 * system.
+		 */
+		sb_start_write(sb);
 		down_read(&sb->s_umount);
 		if (sb->s_root && (sb->s_flags & MS_BORN))
 			f(sb, arg);
 		up_read(&sb->s_umount);
+		sb_end_write(sb);
 
 		spin_lock(&sb_lock);
 		if (p)
diff --git a/fs/sync.c b/fs/sync.c
index fbc98ee..074247f 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -156,9 +156,17 @@ SYSCALL_DEFINE1(syncfs, int, fd)
 		return -EBADF;
 	sb = f.file->f_path.dentry->d_sb;
 
+	/*
+	 * If the file system is frozen we can't proceed because we
+	 * could potentially block on frozen file system. This would
+	 * lead to a deadlock, because we'll be holding s_umount which
+	 * has to be taken in order to thaw the file system as well
+	 */
+	sb_start_write(sb);
 	down_read(&sb->s_umount);
 	ret = sync_filesystem(sb);
 	up_read(&sb->s_umount);
+	sb_end_write(sb);
 
 	fdput(f);
 	return ret;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/