linux-kernel - [PATCH 2/3] ubifs: ubifs_tnc_locate: Drop TNC mutex lockless reading path

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]

Message-ID: <20231120111347.2254153-3-chengzhihao1@huawei.com>
Date:   Mon, 20 Nov 2023 19:13:46 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     <richard@....at>, <Carson.Li1@...soc.com>,
        <s.hauer@...gutronix.de>, <houtao1@...wei.com>,
        <ext-adrian.hunter@...ia.com>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: [PATCH 2/3] ubifs: ubifs_tnc_locate: Drop TNC mutex lockless reading path

This problem is described in [1], and Tao has proposed a solution in [2].
There exists a race between node reading and gc, which is shown as:
         P1                 P2
ubifs_tnc_locate
 zbr.lnum = lnum_a
                         GC // node is moved from lnum_a to lnum_b
			 journal head switching // lnum_a becomes a bud
 ubifs_get_wbuf(c, zbr.lnum) // true
 ubifs_tnc_read_node
  ubifs_read_node_wbuf // read data from lnum_a
  check node failed !

There are two ways of reading node(See ubifs_tnc_read_node()):
1. Reading from wbuf. The node is written in wbuf(in mem), and wbuf is
   not written back to flash.
2. Otherwise, reading from flash.

In most cases, ubifs_tnc_read_node() is invoked with TNC mutex locked,
but except the lockless path in ubifs_tnc_locate() which is imported
by commit 601c0bc46753("UBIFS: allow for racing between GC and TNC").

Function ubifs_tnc_locate() is mainly used for path lookup and file
reading, VFS has inode/dentry/page cache for multiple times reading, the
lockless optimization only works for first reading. Based on the
discussion in [2], this patch simply drops the TNC mutex lockless reading
path in ubifs_tnc_locate().

Fetch a reproducer in [3].

[1] https://lore.kernel.org/all/fda84926-09d1-1fc7-4b78-99e0d04508bc@huawei.com/T/
[2] https://lore.kernel.org/linux-mtd/20200305092205.127758-1-houtao1@huawei.com/
[3] https://bugzilla.kernel.org/show_bug.cgi?id=218163

Fixes: 601c0bc46753 ("UBIFS: allow for racing between GC and TNC")
Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Reported-by: 李傲傲 (Carson Li1/9542) <Carson.Li1@...soc.com>
Analyzed-by: Hou Tao <houtao1@...wei.com>
Signed-off-by: Zhihao Cheng <chengzhihao1@...wei.com>
---
 fs/ubifs/tnc.c | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)

diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index f4728e65d1bd..7b7d75ed3ec7 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -1478,11 +1478,10 @@ static int maybe_leb_gced(struct ubifs_info *c, int lnum, int gc_seq1)
 int ubifs_tnc_locate(struct ubifs_info *c, const union ubifs_key *key,
 		     void *node, int *lnum, int *offs)
 {
-	int found, n, err, safely = 0, gc_seq1;
+	int found, n, err;
 	struct ubifs_znode *znode;
-	struct ubifs_zbranch zbr, *zt;
+	struct ubifs_zbranch *zt;
 
-again:
 	mutex_lock(&c->tnc_mutex);
 	found = ubifs_lookup_level0(c, key, &znode, &n);
 	if (!found) {
@@ -1505,31 +1504,7 @@ int ubifs_tnc_locate(struct ubifs_info *c, const union ubifs_key *key,
 		err = tnc_read_hashed_node(c, zt, node);
 		goto out;
 	}
-	if (safely) {
-		err = ubifs_tnc_read_node(c, zt, node);
-		goto out;
-	}
-	/* Drop the TNC mutex prematurely and race with garbage collection */
-	zbr = znode->zbranch[n];
-	gc_seq1 = c->gc_seq;
-	mutex_unlock(&c->tnc_mutex);
-
-	if (ubifs_get_wbuf(c, zbr.lnum)) {
-		/* We do not GC journal heads */
-		err = ubifs_tnc_read_node(c, &zbr, node);
-		return err;
-	}
-
-	err = fallible_read_node(c, key, &zbr, node);
-	if (err <= 0 || maybe_leb_gced(c, zbr.lnum, gc_seq1)) {
-		/*
-		 * The node may have been GC'ed out from under us so try again
-		 * while keeping the TNC mutex locked.
-		 */
-		safely = 1;
-		goto again;
-	}
-	return 0;
+	err = ubifs_tnc_read_node(c, zt, node);
 
 out:
 	mutex_unlock(&c->tnc_mutex);
-- 
2.39.2