diff mbox series

[v3,2/2] ubifs: read node from wbuf when it fully sits in wbuf

Message ID 20200305092205.127758-3-houtao1@huawei.com
State New
Delegated to: Richard Weinberger
Headers show
Series fix potential race between ubifs_tnc_locate() and GC | expand

Commit Message

Hou Tao March 5, 2020, 9:22 a.m. UTC
Carson Li Reports the following error:

 UBIFS error: ubifs_read_node_wbuf: expected node type 0
 Not a node, first 24 bytes:
 Kernel panic - not syncing
 CPU: 1 PID: 943 Comm: http-thread 4.4.83 #1
   panic+0x70/0x1e4
   ubifs_dump_node+0x6c/0x9a0
   ubifs_read_node_wbuf+0x350/0x384
   ubifs_tnc_read_node+0x54/0x214
   ubifs_tnc_locate+0x118/0x1b4
   ubifs_iget+0xb8/0x68c
   ubifs_lookup+0x1b4/0x258
   lookup_real+0x30/0x4c
   __lookup_hash+0x34/0x3c
   walk_component+0xec/0x2a0
   path_lookupat+0x80/0xfc
   filename_lookup+0x5c/0xfc
   vfs_fstatat+0x4c/0x9c
   SyS_stat64+0x14/0x30
   ret_fast_syscall+0x0/0x34

It seems the LEB used as DATA journal head is GC'ed, and ubifs_tnc_locate()
read an invalid node. But now the property of journal head LEB has
LPROPS_TAKEN flag set and GC will skip these LEBs.

The actual situation of the problem is the LEB is GCed, freed and then
reused as journal head, and finally ubifs_tnc_locate() reads
an invalid node. And it can be reproduced by the following steps:
* create 128 empty files
* overwrite 8 files in backgroup repeatedly to trigger GC
* drop inode cache and stat these 128 empty files repeatedly

We can simply fix the problem by removing the optimization of reading
wbuf when possible. But because taking spin lock and memcpying from
wbuf is much less time-consuming than reading from MTD device, so we fix
the logic of wbuf reading instead.

If the node is not fully contained in write buffer, we will try to
reading the remained node from MTD without any lock, and the
journal head may be switched and GCed, and we will get invalid
node data. So we only read from wbuf if the node fully sits in
the write buffer.

And we also need to check whether or not the current is LEB is GC'ed
and reused as journal head.

Link: https://www.spinics.net/lists/linux-mtd/msg10771.html
Fixes: 601c0bc46753 ("UBIFS: allow for racing between GC and TNC")
Reported-and-analyzed-by: 李傲傲 (Carson Li1/9542) <Carson.Li1@unisoc.com>
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/ubifs/tnc.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 3 deletions(-)

Comments

Richard Weinberger March 16, 2020, 10:21 p.m. UTC | #1
Hou Tao,

On Thu, Mar 5, 2020 at 10:15 AM Hou Tao <houtao1@huawei.com> wrote:
>
> Carson Li Reports the following error:
>
>  UBIFS error: ubifs_read_node_wbuf: expected node type 0
>  Not a node, first 24 bytes:
>  Kernel panic - not syncing
>  CPU: 1 PID: 943 Comm: http-thread 4.4.83 #1
>    panic+0x70/0x1e4
>    ubifs_dump_node+0x6c/0x9a0
>    ubifs_read_node_wbuf+0x350/0x384
>    ubifs_tnc_read_node+0x54/0x214
>    ubifs_tnc_locate+0x118/0x1b4
>    ubifs_iget+0xb8/0x68c
>    ubifs_lookup+0x1b4/0x258
>    lookup_real+0x30/0x4c
>    __lookup_hash+0x34/0x3c
>    walk_component+0xec/0x2a0
>    path_lookupat+0x80/0xfc
>    filename_lookup+0x5c/0xfc
>    vfs_fstatat+0x4c/0x9c
>    SyS_stat64+0x14/0x30
>    ret_fast_syscall+0x0/0x34
>
> It seems the LEB used as DATA journal head is GC'ed, and ubifs_tnc_locate()
> read an invalid node. But now the property of journal head LEB has
> LPROPS_TAKEN flag set and GC will skip these LEBs.
>
> The actual situation of the problem is the LEB is GCed, freed and then
> reused as journal head, and finally ubifs_tnc_locate() reads
> an invalid node. And it can be reproduced by the following steps:
> * create 128 empty files
> * overwrite 8 files in backgroup repeatedly to trigger GC
> * drop inode cache and stat these 128 empty files repeatedly
>
> We can simply fix the problem by removing the optimization of reading
> wbuf when possible. But because taking spin lock and memcpying from
> wbuf is much less time-consuming than reading from MTD device, so we fix
> the logic of wbuf reading instead.

I'm digging now into that issue. Did you also experiment with reading while
tnc_mutex is locked? So, no race at all (having safely = 1 by default).
Just to make sure we don't fix an no longer needed optimization.

The code is already anything but trivial and adding more code makes me
nervous.
Richard Weinberger March 16, 2020, 10:33 p.m. UTC | #2
On Thu, Mar 5, 2020 at 10:15 AM Hou Tao <houtao1@huawei.com> wrote:
> The actual situation of the problem is the LEB is GCed, freed and then
> reused as journal head, and finally ubifs_tnc_locate() reads
> an invalid node. And it can be reproduced by the following steps:
> * create 128 empty files
> * overwrite 8 files in backgroup repeatedly to trigger GC
> * drop inode cache and stat these 128 empty files repeatedly

So far I failed to reproduce. Do you have a script?
Or even better, a xfstest?
Hou Tao March 18, 2020, 1:57 a.m. UTC | #3
Hi,

On 2020/3/17 6:33, Richard Weinberger wrote:
> On Thu, Mar 5, 2020 at 10:15 AM Hou Tao <houtao1@huawei.com> wrote:
>> The actual situation of the problem is the LEB is GCed, freed and then
>> reused as journal head, and finally ubifs_tnc_locate() reads
>> an invalid node. And it can be reproduced by the following steps:
>> * create 128 empty files
>> * overwrite 8 files in backgroup repeatedly to trigger GC
>> * drop inode cache and stat these 128 empty files repeatedly
> 
> So far I failed to reproduce. Do you have a script?
> Or even better, a xfstest?
> 
You can increase the probability by adding an extra delay (e.g. msleep(1))
between the unlock of tnc_mutex and the call of ubifs_get_wbuf().
And I will try to writ xfstest for the problem.

Regards,
Tao
Hou Tao July 1, 2020, 1:15 a.m. UTC | #4
Hi,

On 2020/3/17 6:21, Richard Weinberger wrote:
> Hou Tao,
> 
> On Thu, Mar 5, 2020 at 10:15 AM Hou Tao <houtao1@huawei.com> wrote:
>>
>> Carson Li Reports the following error:
>>
>>  UBIFS error: ubifs_read_node_wbuf: expected node type 0
>>  Not a node, first 24 bytes:
>>  Kernel panic - not syncing
>>  CPU: 1 PID: 943 Comm: http-thread 4.4.83 #1
>>    panic+0x70/0x1e4
>>    ubifs_dump_node+0x6c/0x9a0
>>    ubifs_read_node_wbuf+0x350/0x384
>>    ubifs_tnc_read_node+0x54/0x214
>>    ubifs_tnc_locate+0x118/0x1b4
>>    ubifs_iget+0xb8/0x68c
>>    ubifs_lookup+0x1b4/0x258
>>    lookup_real+0x30/0x4c
>>    __lookup_hash+0x34/0x3c
>>    walk_component+0xec/0x2a0
>>    path_lookupat+0x80/0xfc
>>    filename_lookup+0x5c/0xfc
>>    vfs_fstatat+0x4c/0x9c
>>    SyS_stat64+0x14/0x30
>>    ret_fast_syscall+0x0/0x34
>>
>> It seems the LEB used as DATA journal head is GC'ed, and ubifs_tnc_locate()
>> read an invalid node. But now the property of journal head LEB has
>> LPROPS_TAKEN flag set and GC will skip these LEBs.
>>
>> The actual situation of the problem is the LEB is GCed, freed and then
>> reused as journal head, and finally ubifs_tnc_locate() reads
>> an invalid node. And it can be reproduced by the following steps:
>> * create 128 empty files
>> * overwrite 8 files in backgroup repeatedly to trigger GC
>> * drop inode cache and stat these 128 empty files repeatedly
>>
>> We can simply fix the problem by removing the optimization of reading
>> wbuf when possible. But because taking spin lock and memcpying from
>> wbuf is much less time-consuming than reading from MTD device, so we fix
>> the logic of wbuf reading instead.
> 
> I'm digging now into that issue. Did you also experiment with reading while
> tnc_mutex is locked? So, no race at all (having safely = 1 by default).
> Just to make sure we don't fix an no longer needed optimization.
>> The code is already anything but trivial and adding more code makes me
> nervous.
> 
Sorry for the late reply. I'm fine if we remove the read-wbuf optimization,
but I need to check the code firstly, running the reproducing program, and lastly
writing a xfstest for it.

Regards,
Tao
diff mbox series

Patch

diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index e8e7b0e9532e..d4c0435d0276 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -1425,6 +1425,74 @@  static int maybe_leb_gced(struct ubifs_info *c, int lnum, int gc_seq1)
 	return 0;
 }
 
+/**
+ * ubifs_check_and_read_wbuf - read node from write-buffer if possible
+ * @c: UBIFS file-system description object
+ * @zbr: the zbranch describing the node to read
+ * @gc_seq: the saved GC sequence used for GC checking
+ * @buf: buffer to read to
+ * @retry: whether try to lookup TNC again
+ *
+ * The function checks whether the node fully sits in the write-buffer
+ * and whether the LEB used by write-buffer is not GCed recently,
+ * then it will read the node, checks it and stores in @buf.
+ *
+ * Returns 1 in case of success, 0 in case of not found, and a negative
+ * error code in case of failure.
+ *
+ * If the node is not in write-buffer and the LEB used by write-buffer
+ * may be GCed recently, @retry will be true, else false.
+ */
+static int ubifs_check_and_read_wbuf(struct ubifs_info *c,
+				     const struct ubifs_zbranch *zbr,
+				     int gc_seq, void *buf, bool *retry)
+{
+	bool found = false;
+	int lnum = zbr->lnum;
+	int offs = zbr->offs;
+	int len = zbr->len;
+	int type;
+	int i;
+	int err;
+
+	*retry = false;
+	for (i = 0; i < c->jhead_cnt; i++) {
+		struct ubifs_wbuf *wbuf = &c->jheads[i].wbuf;
+
+		/* Check whether the node is fully included in wbuf */
+		spin_lock(&wbuf->lock);
+		if (wbuf->lnum == lnum && wbuf->offs <= offs &&
+		    offs + len <= wbuf->offs + wbuf->used) {
+			/*
+			 * lnum is GC'ed and reused as journal head,
+			 * we need to lookup TNC again.
+			 */
+			if (maybe_leb_gced(c, lnum, gc_seq)) {
+				spin_unlock(&wbuf->lock);
+				*retry = true;
+				break;
+			}
+
+			memcpy(buf, wbuf->buf + offs - wbuf->offs, len);
+			spin_unlock(&wbuf->lock);
+			found = true;
+			break;
+		}
+		spin_unlock(&wbuf->lock);
+	}
+
+	if (!found)
+		return 0;
+
+	type = key_type(c, &zbr->key);
+	err = ubifs_check_node_buf(c, buf, type, len, lnum, offs,
+				   UBIFS_CHK_FORCE_DUMP_BAD_NODE);
+	if (err)
+		return err;
+
+	return 1;
+}
+
 /**
  * ubifs_tnc_locate - look up a file-system node and return it and its location.
  * @c: UBIFS file-system description object
@@ -1444,6 +1512,7 @@  int ubifs_tnc_locate(struct ubifs_info *c, const union ubifs_key *key,
 	int found, n, err, safely = 0, gc_seq1;
 	struct ubifs_znode *znode;
 	struct ubifs_zbranch zbr, *zt;
+	bool retry;
 
 again:
 	mutex_lock(&c->tnc_mutex);
@@ -1477,10 +1546,16 @@  int ubifs_tnc_locate(struct ubifs_info *c, const union ubifs_key *key,
 	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);
+	err = ubifs_check_and_read_wbuf(c, &zbr, gc_seq1, node, &retry);
+	if (err < 0)
 		return err;
+	/* find a valid node */
+	if (err > 0)
+		return 0;
+	/* The node is GC'ed, so lookup it again */
+	if (retry) {
+		safely = 1;
+		goto again;
 	}
 
 	err = fallible_read_node(c, key, &zbr, node);