| Message ID | 20200305092205.127758-3-houtao1@huawei.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | fix potential race between ubifs_tnc_locate() and GC | expand |
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.
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?
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
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 --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);
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(-)