Re: ubifs: read bad node type in ubifs_tnc_read_wbuf

[Hou Tao <houtao1@xxxxxxxxxx>]

Hi,

On 2020/3/2 11:12, 李傲傲 (Carson Li1/9542) wrote:
> Hi,
> 
>> 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.
> 
> Actually, I think that situation might only be caused by a commit, is that right?
> Since only commit might clear the journal head LEBs' property of LPROPS_TAKEN.
> But it will not get the c->jheads[i].wbuf->lnum LEB's taken property cleared, so there
> seems no need to check if a c->jheads[i].wbuf->lnum LEB might be GCed and the
> node whether fully sits in wirte buffer.
No. The GC'ed checking is needed here. As I have noted in the commit message:

>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

In the above steps, the nodes related with these empty files are already been committed
long before the running of stat command.

> 
> but it is another situation if there is not only one but two or more commits happen,
> the LEB with jheads[i].wbuf->lnum should be considered whether it has been GCed.
> I am not sure if we need to get such a less possible situation into account :( .
> 
>> +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);
>> +}
>> +
> 
> Is it more safely to have may_leb_gced after memcpy though memcpy is quickly?

Do you mean call may_leb_gced() after the release of wbuf->lock ? If it's the case,
it's OK to me because it will reduce the hold time of wbuf->lock.

> 
>> +if (!found)
> +return 0;
> +
> +type = key_type(c, &zbr->key);
> +err = ubifs_check_node_buf(c, buf, type, len, lnum, offs);
> +if (err)
> +return err;
> +
> +return 1;
> +}
> +
> 
> I don't find the ubifs_check_node_buf , is it a function you newly defined in this patch?
> 
It's in the first patch and I have not attached it here. It's a helper function factored out
from ubifs_read_node_wbuf() and is used to check the validity of node in buffer:

Subject: [PATCH v2 1/2] ubifs: factor out helper ubifs_check_node_buf()

It will be used by the following patch to check
the validity of node in buf.

Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
---
 fs/ubifs/io.c    | 59 +++++++++++++++++++++++++++++-------------------
 fs/ubifs/ubifs.h |  2 ++
 2 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 8ceb51478800..6566bb5ffd58 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -946,6 +946,40 @@ int ubifs_write_node(struct ubifs_info *c, void *buf, int len, int lnum,
 	return ubifs_write_node_hmac(c, buf, len, lnum, offs, -1);
 }

+int ubifs_check_node_buf(const struct ubifs_info *c, void *buf, int type,
+			 int len, int lnum, int offs)
+
+{
+	const struct ubifs_ch *ch = buf;
+	int err;
+	int ch_len;
+
+	if (type != ch->node_type) {
+		ubifs_err(c, "bad node type (%d but expected %d)",
+			  ch->node_type, type);
+		goto out;
+	}
+
+	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
+	if (err) {
+		ubifs_err(c, "expected node type %d", type);
+		return err;
+	}
+
+	ch_len = le32_to_cpu(ch->len);
+	if (ch_len != len) {
+		ubifs_err(c, "bad node length %d, expected %d", ch_len, len);
+		goto out;
+	}
+
+	return 0;
+out:
+	ubifs_errc(c, "bad node at LEB %d:%d", lnum, offs);
+	ubifs_dump_node(c, buf);
+	dump_stack();
+	return -EINVAL;
+}
+
 /**
  * ubifs_read_node_wbuf - read node from the media or write-buffer.
  * @wbuf: wbuf to check for un-written data
@@ -966,7 +1000,6 @@ int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 {
 	const struct ubifs_info *c = wbuf->c;
 	int err, rlen, overlap;
-	struct ubifs_ch *ch = buf;

 	dbg_io("LEB %d:%d, %s, length %d, jhead %s", lnum, offs,
 	       dbg_ntype(type), len, dbg_jhead(wbuf->jhead));
@@ -998,31 +1031,11 @@ int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 			return err;
 	}

-	if (type != ch->node_type) {
-		ubifs_err(c, "bad node type (%d but expected %d)",
-			  ch->node_type, type);
-		goto out;
-	}
-
-	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
-	if (err) {
-		ubifs_err(c, "expected node type %d", type);
+	err = ubifs_check_node_buf(c, buf, type, len, lnum, offs);
+	if (err)
 		return err;
-	}
-
-	rlen = le32_to_cpu(ch->len);
-	if (rlen != len) {
-		ubifs_err(c, "bad node length %d, expected %d", rlen, len);
-		goto out;
-	}

 	return 0;
-
-out:
-	ubifs_err(c, "bad node at LEB %d:%d", lnum, offs);
-	ubifs_dump_node(c, buf);
-	dump_stack();
-	return -EINVAL;
 }

 /**
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index bff682309fbe..6ae4e8f2131d 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1710,6 +1710,8 @@ int ubifs_is_mapped(const struct ubifs_info *c, int lnum);
 int ubifs_wbuf_write_nolock(struct ubifs_wbuf *wbuf, void *buf, int len);
 int ubifs_wbuf_seek_nolock(struct ubifs_wbuf *wbuf, int lnum, int offs);
 int ubifs_wbuf_init(struct ubifs_info *c, struct ubifs_wbuf *wbuf);
+int ubifs_check_node_buf(const struct ubifs_info *c, void *buf, int type,
+			 int len, int lnum, int offs);
 int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		    int lnum, int offs);
 int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
-- 
2.25.0.4.g0ad7144999

Regards,
Tao

> Thanks.
> Carson
> 
> ________________________________
>  This email (including its attachments) is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. Unauthorized use, dissemination, distribution or copying of this email or the information herein or taking any action in reliance on the contents of this email or the information herein, by anyone other than the intended recipient, or an employee or agent responsible for delivering the message to the intended recipient, is strictly prohibited. If you are not the intended recipient, please do not read, copy, use or disclose any part of this e-mail to others. Please notify the sender immediately and permanently delete this e-mail and any attachments if you received it in error. Internet communications cannot be guaranteed to be timely, secure, error-free or virus-free. The sender does not accept liability for any errors or omissions.
> 本邮件及其附件具有保密性质，受法律保护不得泄露，仅发送给本邮件所指特定收件人。严禁非经授权使用、宣传、发布或复制本邮件或其内容。若非该特定收件人，请勿阅读、复制、 使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件的方式即刻告知发件人。无法保证互联网通信及时、安全、无误或防毒。发件人对任何错漏均不承担责任。
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/