linux-kernel - [PATCH] fs/jbd2: fix data races in jbd2_journal_set

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: <1442833039-55373-1-git-send-email-dvyukov@google.com>
Date:	Mon, 21 Sep 2015 12:57:19 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	tytso@....edu, jack@...e.com, linux-ext4@...r.kernel.org,
	linux-kernel@...r.kernel.org
Cc:	kcc@...gle.com, glider@...gle.com, andreyknvl@...gle.com,
	ktsan@...glegroups.com, paulmck@...ux.vnet.ibm.com,
	Dmitry Vyukov <dvyukov@...gle.com>
Subject: [PATCH] fs/jbd2: fix data races in jbd2_journal_set_features

jbd2_journal_set_features() uses complex logic to update
various feature flags while they are being concurrently read.
This gives compiler whole lot of possibilities to temporary
set features to unexpected values and break readers.
For example, compiler can speculatively set or reset some flag
and then restore the old value if assumptions does not hold.

Use WRITE_ONCE() to update feature flags in jbd2_journal_set_features().

The data race was found with KernelThreadSanitizer (KTSAN).

Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
---
For the record here is KTSAN report on 4.2 rc2:

ThreadSanitizer: data-race in journal_tag_bytes

Read at 0xffff8800bba31028 of size 4 by thread 961 on CPU 6:
 [<ffffffff813c9982>] journal_tag_bytes+0x42/0x90 fs/jbd2/journal.c:2191 (discriminator 1)
 [<ffffffff813ba682>] jbd2_journal_commit_transaction+0x42/0x3190 fs/jbd2/commit.c:390
 [<ffffffff813c698b>] kjournald2+0x13b/0x430 fs/jbd2/journal.c:223
 [<ffffffff810bba40>] kthread+0x150/0x170 kernel/kthread.c:209
 [<ffffffff81ee420f>] ret_from_fork+0x3f/0x70 arch/x86/entry/entry_64.S:529

Previous write at 0xffff8800bba31028 of size 4 by thread 2865 on CPU 2:
 [<ffffffff813c7406>] jbd2_journal_set_features+0x166/0x370 fs/jbd2/journal.c:1872
 [<ffffffff813c106b>] jbd2_journal_revoke+0x4b/0x1b0 fs/jbd2/revoke.c:337
 [<ffffffff8138e2fe>] __ext4_forget+0x13e/0x240 fs/ext4/ext4_jbd2.c:228
 [<ffffffff8139d778>] ext4_free_blocks+0x168/0x10f0 fs/ext4/mballoc.c:4698
 [<ffffffff81381a01>] ext4_ext_rm_idx+0x1e1/0x340 fs/ext4/extents.c:2385
 [<     inline     >] ext4_ext_rm_leaf fs/ext4/extents.c:2771
 [<ffffffff81387844>] ext4_ext_remove_space+0x1604/0x1ad0 fs/ext4/extents.c:2932
 [<ffffffff8138b07f>] ext4_ext_truncate+0xcf/0x150 fs/ext4/extents.c:4656
 [<ffffffff81341bdf>] ext4_truncate+0x64f/0x690 fs/ext4/inode.c:3767
 [<ffffffff81342bff>] ext4_evict_inode+0x63f/0x6f0 fs/ext4/inode.c:261
 [<ffffffff8128d2e0>] evict+0x180/0x2c0 fs/inode.c:544
 [<     inline     >] iput_final fs/inode.c:1465
 [<ffffffff8128e6b4>] iput+0x274/0x3c0 fs/inode.c:1492
 [<ffffffff8127b1a6>] do_unlinkat+0x2b6/0x4a0 fs/namei.c:3864
 [<     inline     >] SYSC_unlink fs/namei.c:3905
 [<ffffffff8127c172>] SyS_unlink+0x22/0x40 fs/namei.c:3903
 [<ffffffff81ee3e11>] entry_SYSCALL_64_fastpath+0x31/0x95 arch/x86/entry/entry_64.S:188

Mutexes locked by thread 2865:
Mutex 469497 is locked here:
 [<ffffffff81ee0d45>] down_write+0x65/0x80 kernel/locking/rwsem.c:62
 [<ffffffff81341b09>] ext4_truncate+0x579/0x690 fs/ext4/inode.c:3762
 [<ffffffff81342bff>] ext4_evict_inode+0x63f/0x6f0 fs/ext4/inode.c:261
 [<ffffffff8128d2e0>] evict+0x180/0x2c0 fs/inode.c:544
 [<     inline     >] iput_final fs/inode.c:1465
 [<ffffffff8128e6b4>] iput+0x274/0x3c0 fs/inode.c:1492
 [<ffffffff8127b1a6>] do_unlinkat+0x2b6/0x4a0 fs/namei.c:3864
 [<     inline     >] SYSC_unlink fs/namei.c:3905
 [<ffffffff8127c172>] SyS_unlink+0x22/0x40 fs/namei.c:3903
 [<ffffffff81ee3e11>] entry_SYSCALL_64_fastpath+0x31/0x95 arch/x86/entry/entry_64.S:188
---
 fs/jbd2/journal.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 8270fe9..cfc5127 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1849,8 +1849,8 @@ int jbd2_journal_set_features (journal_t *journal, unsigned long compat,
 	/* If enabling v3 checksums, update superblock */
 	if (INCOMPAT_FEATURE_ON(JBD2_FEATURE_INCOMPAT_CSUM_V3)) {
 		sb->s_checksum_type = JBD2_CRC32C_CHKSUM;
-		sb->s_feature_compat &=
-			~cpu_to_be32(JBD2_FEATURE_COMPAT_CHECKSUM);
+		WRITE_ONCE(sb->s_feature_compat, sb->s_feature_compat &
+			~cpu_to_be32(JBD2_FEATURE_COMPAT_CHECKSUM));
 
 		/* Load the checksum driver */
 		if (journal->j_chksum_driver == NULL) {
@@ -1872,13 +1872,16 @@ int jbd2_journal_set_features (journal_t *journal, unsigned long compat,
 
 	/* If enabling v1 checksums, downgrade superblock */
 	if (COMPAT_FEATURE_ON(JBD2_FEATURE_COMPAT_CHECKSUM))
-		sb->s_feature_incompat &=
+		WRITE_ONCE(sb->s_feature_incompat, sb->s_feature_incompat &
 			~cpu_to_be32(JBD2_FEATURE_INCOMPAT_CSUM_V2 |
-				     JBD2_FEATURE_INCOMPAT_CSUM_V3);
-
-	sb->s_feature_compat    |= cpu_to_be32(compat);
-	sb->s_feature_ro_compat |= cpu_to_be32(ro);
-	sb->s_feature_incompat  |= cpu_to_be32(incompat);
+				     JBD2_FEATURE_INCOMPAT_CSUM_V3));
+
+	WRITE_ONCE(sb->s_feature_compat,
+		sb->s_feature_compat | cpu_to_be32(compat));
+	WRITE_ONCE(sb->s_feature_ro_compat,
+		sb->s_feature_ro_compat | cpu_to_be32(ro));
+	WRITE_ONCE(sb->s_feature_incompat,
+		sb->s_feature_incompat | cpu_to_be32(incompat));
 
 	return 1;
 #undef COMPAT_FEATURE_ON
-- 
2.6.0.rc0.131.gf624c3d

--
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/