linux-kernel - [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw

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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 26 Sep 2016 18:55:25 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Dave Chinner <david@...morbit.com>,
        Nikolay Borisov <kernel@...p.com>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and
 thaw_super() paths

On 09/26, Jan Kara wrote:
>
> On Mon 26-09-16 18:08:06, Oleg Nesterov wrote:
> > +/*
> > + * Tell lockdep we are holding these locks before we call ->unfreeze_fs(sb).
> > + */
> > +static void sb_freeze_acquire(struct super_block *sb)
>
> Can we call this lockdep_sb_freeze_acquire() or something like that so that
> it is clear this is only about lockdep annotations? Similarly with
> sb_freeze_unlock()...

OK, thanks, done. See V2 below.

> and I hope you really tested
> there are no more lockdep false positives ;).

Heh ;) if only I knew how to test this... I ran the following script under qemu

	mkfs.xfs -f /dev/vda
	mkfs.xfs -f /dev/vdb

	mkdir -p TEST SCRATCH

	TEST_DEV=/dev/vda TEST_DIR=TEST SCRATCH_DEV=/dev/vdb SCRATCH_MNT=SCRATCH \
	./check `grep -il freeze tests/*/???`

this time.

And yes, I'm afraid this change can uncover some false positives later. But at
the same time potentially it can find the real problems.

It would be nice to remove another hack in __sb_start_write under ifdef(CONFIG_LOCKDEP),
but iirc XFS actually takes the same rw_sem twice for reading, so we can't do this.

-------------------------------------------------------------------------------
>From 70e774533ab6319f9b90a490b036150ad9604af7 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@...hat.com>
Date: Mon, 26 Sep 2016 17:23:24 +0200
Subject: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw_super() paths

sb_wait_write()->percpu_rwsem_release() fools lockdep to avoid the
false-positives. Now that xfs was fixed by Dave's commit dbad7c993053
("xfs: stop holding ILOCK over filldir callbacks") we can remove it and
change freeze_super() and thaw_super() to run with s_writers.rw_sem locks
held; we add two trivial helpers for that, lockdep_sb_freeze_release()
and lockdep_sb_freeze_acquire().

xfstests-dev/check `grep -il freeze tests/*/???` does not trigger any
warning from lockdep.

Signed-off-by: Oleg Nesterov <oleg@...hat.com>
---
 fs/super.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 2549896c..0447afe 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1214,25 +1214,34 @@ EXPORT_SYMBOL(__sb_start_write);
 static void sb_wait_write(struct super_block *sb, int level)
 {
 	percpu_down_write(sb->s_writers.rw_sem + level-1);
-	/*
-	 * We are going to return to userspace and forget about this lock, the
-	 * ownership goes to the caller of thaw_super() which does unlock.
-	 *
-	 * FIXME: we should do this before return from freeze_super() after we
-	 * called sync_filesystem(sb) and s_op->freeze_fs(sb), and thaw_super()
-	 * should re-acquire these locks before s_op->unfreeze_fs(sb). However
-	 * this leads to lockdep false-positives, so currently we do the early
-	 * release right after acquire.
-	 */
-	percpu_rwsem_release(sb->s_writers.rw_sem + level-1, 0, _THIS_IP_);
 }
 
-static void sb_freeze_unlock(struct super_block *sb)
+/*
+ * We are going to return to userspace and forget about these locks, the
+ * ownership goes to the caller of thaw_super() which does unlock().
+ */
+static void lockdep_sb_freeze_release(struct super_block *sb)
+{
+	int level;
+
+	for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
+		percpu_rwsem_release(sb->s_writers.rw_sem + level, 0, _THIS_IP_);
+}
+
+/*
+ * Tell lockdep we are holding these locks before we call ->unfreeze_fs(sb).
+ */
+static void lockdep_sb_freeze_acquire(struct super_block *sb)
 {
 	int level;
 
 	for (level = 0; level < SB_FREEZE_LEVELS; ++level)
 		percpu_rwsem_acquire(sb->s_writers.rw_sem + level, 0, _THIS_IP_);
+}
+
+static void sb_freeze_unlock(struct super_block *sb)
+{
+	int level;
 
 	for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--)
 		percpu_up_write(sb->s_writers.rw_sem + level);
@@ -1328,6 +1337,7 @@ int freeze_super(struct super_block *sb)
 	 * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
 	 */
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
+	lockdep_sb_freeze_release(sb);
 	up_write(&sb->s_umount);
 	return 0;
 }
@@ -1354,11 +1364,14 @@ int thaw_super(struct super_block *sb)
 		goto out;
 	}
 
+	lockdep_sb_freeze_acquire(sb);
+
 	if (sb->s_op->unfreeze_fs) {
 		error = sb->s_op->unfreeze_fs(sb);
 		if (error) {
 			printk(KERN_ERR
 				"VFS:Filesystem thaw failed\n");
+			lockdep_sb_freeze_release(sb);
 			up_write(&sb->s_umount);
 			return error;
 		}
-- 
2.5.0