| Message ID | 20230215022507.2363193-1-chengzhihao1@huawei.com |
|---|---|
| State | Superseded |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | [v2] ubifs: dirty_cow_znode: Fix memleak in error handling path | expand |
在 2023/2/15 10:25, Zhihao Cheng 写道: Hi, Richarch, > Following process will cause a memleak for copied up znode: > > dirty_cow_znode > zn = copy_znode(c, znode); > err = insert_old_idx(c, zbr->lnum, zbr->offs); > if (unlikely(err)) > return ERR_PTR(err); // No one refers to zn. > > Fetch a reproducer in [Link]. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216705 > Fixes: 1e51764a3c2a ("UBIFS: add new flash file system") > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> > --- > v1: https://patchwork.ozlabs.org/project/linux-mtd/patch/20221118090236.664244-3-chengzhihao1@huawei.com/ > v1->v2: In v1, if insert_old_idx() failed, old index neither exists in > TNC nor in old-index tree. Which means that old index node could > be overwritten in layout_leb_in_gaps(), then ubifs image will be > corrupted in power-cut. > In v2, copy_znode() is splitted into 2 parts: resouce allocation > and znode replacement, insert_old_idx() is splitted in similar > way, so resouce cleanup could be done in error handling path > without corrupting metadata(mem & disk). > It's okay that old index inserting is put behind of add_idx_dirt(), > old index is used in layout_leb_in_gaps(), so the two processes > do not depend on each other. > fs/ubifs/tnc.c | 137 +++++++++++++++++++++++++++++++------------------ > 1 file changed, 87 insertions(+), 50 deletions(-) > Just a reminder to notify that this is a v2 fix to replace original solution (which is in next branch) https://patchwork.ozlabs.org/project/linux-mtd/patch/20221118090236.664244-3-chengzhihao1@huawei.com/. > diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c > index 2df56bbc6865..6b7d95b65f4b 100644 > --- a/fs/ubifs/tnc.c > +++ b/fs/ubifs/tnc.c > @@ -44,6 +44,33 @@ enum { > NOT_ON_MEDIA = 3, > }; > > +static void do_insert_old_idx(struct ubifs_info *c, > + struct ubifs_old_idx *old_idx) > +{ > + struct ubifs_old_idx *o; > + struct rb_node **p, *parent = NULL; > + > + p = &c->old_idx.rb_node; > + while (*p) { > + parent = *p; > + o = rb_entry(parent, struct ubifs_old_idx, rb); > + if (old_idx->lnum < o->lnum) > + p = &(*p)->rb_left; > + else if (old_idx->lnum > o->lnum) > + p = &(*p)->rb_right; > + else if (old_idx->offs < o->offs) > + p = &(*p)->rb_left; > + else if (old_idx->offs > o->offs) > + p = &(*p)->rb_right; > + else { > + ubifs_err(c, "old idx added twice!"); > + kfree(old_idx); > + } > + } > + rb_link_node(&old_idx->rb, parent, p); > + rb_insert_color(&old_idx->rb, &c->old_idx); > +} > + > /** > * insert_old_idx - record an index node obsoleted since the last commit start. > * @c: UBIFS file-system description object > @@ -69,35 +96,15 @@ enum { > */ > static int insert_old_idx(struct ubifs_info *c, int lnum, int offs) > { > - struct ubifs_old_idx *old_idx, *o; > - struct rb_node **p, *parent = NULL; > + struct ubifs_old_idx *old_idx; > > old_idx = kmalloc(sizeof(struct ubifs_old_idx), GFP_NOFS); > if (unlikely(!old_idx)) > return -ENOMEM; > old_idx->lnum = lnum; > old_idx->offs = offs; > + do_insert_old_idx(c, old_idx); > > - p = &c->old_idx.rb_node; > - while (*p) { > - parent = *p; > - o = rb_entry(parent, struct ubifs_old_idx, rb); > - if (lnum < o->lnum) > - p = &(*p)->rb_left; > - else if (lnum > o->lnum) > - p = &(*p)->rb_right; > - else if (offs < o->offs) > - p = &(*p)->rb_left; > - else if (offs > o->offs) > - p = &(*p)->rb_right; > - else { > - ubifs_err(c, "old idx added twice!"); > - kfree(old_idx); > - return 0; > - } > - } > - rb_link_node(&old_idx->rb, parent, p); > - rb_insert_color(&old_idx->rb, &c->old_idx); > return 0; > } > > @@ -199,23 +206,6 @@ static struct ubifs_znode *copy_znode(struct ubifs_info *c, > __set_bit(DIRTY_ZNODE, &zn->flags); > __clear_bit(COW_ZNODE, &zn->flags); > > - ubifs_assert(c, !ubifs_zn_obsolete(znode)); > - __set_bit(OBSOLETE_ZNODE, &znode->flags); > - > - if (znode->level != 0) { > - int i; > - const int n = zn->child_cnt; > - > - /* The children now have new parent */ > - for (i = 0; i < n; i++) { > - struct ubifs_zbranch *zbr = &zn->zbranch[i]; > - > - if (zbr->znode) > - zbr->znode->parent = zn; > - } > - } > - > - atomic_long_inc(&c->dirty_zn_cnt); > return zn; > } > > @@ -233,6 +223,42 @@ static int add_idx_dirt(struct ubifs_info *c, int lnum, int dirt) > return ubifs_add_dirt(c, lnum, dirt); > } > > +/** > + * replace_znode - replace old znode with new znode. > + * @c: UBIFS file-system description object > + * @new_zn: new znode > + * @old_zn: old znode > + * @zbr: the branch of parent znode > + * > + * Replace old znode with new znode in TNC. > + */ > +static void replace_znode(struct ubifs_info *c, struct ubifs_znode *new_zn, > + struct ubifs_znode *old_zn, struct ubifs_zbranch *zbr) > +{ > + ubifs_assert(c, !ubifs_zn_obsolete(old_zn)); > + __set_bit(OBSOLETE_ZNODE, &old_zn->flags); > + > + if (old_zn->level != 0) { > + int i; > + const int n = new_zn->child_cnt; > + > + /* The children now have new parent */ > + for (i = 0; i < n; i++) { > + struct ubifs_zbranch *child = &new_zn->zbranch[i]; > + > + if (child->znode) > + child->znode->parent = new_zn; > + } > + } > + > + zbr->znode = new_zn; > + zbr->lnum = 0; > + zbr->offs = 0; > + zbr->len = 0; > + > + atomic_long_inc(&c->dirty_zn_cnt); > +} > + > /** > * dirty_cow_znode - ensure a znode is not being committed. > * @c: UBIFS file-system description object > @@ -265,21 +291,32 @@ static struct ubifs_znode *dirty_cow_znode(struct ubifs_info *c, > return zn; > > if (zbr->len) { > - err = insert_old_idx(c, zbr->lnum, zbr->offs); > - if (unlikely(err)) > - return ERR_PTR(err); > + struct ubifs_old_idx *old_idx; > + > + old_idx = kmalloc(sizeof(struct ubifs_old_idx), GFP_NOFS); > + if (unlikely(!old_idx)) { > + err = -ENOMEM; > + goto out; > + } > + old_idx->lnum = zbr->lnum; > + old_idx->offs = zbr->offs; > + > err = add_idx_dirt(c, zbr->lnum, zbr->len); > - } else > - err = 0; > + if (err) { > + kfree(old_idx); > + goto out; > + } > > - zbr->znode = zn; > - zbr->lnum = 0; > - zbr->offs = 0; > - zbr->len = 0; > + do_insert_old_idx(c, old_idx); > + } > + > + replace_znode(c, zn, znode, zbr); > > - if (unlikely(err)) > - return ERR_PTR(err); > return zn; > + > +out: > + kfree(zn); > + return ERR_PTR(err); > } > > /** >
----- Ursprüngliche Mail ----- > Von: "chengzhihao1" <chengzhihao1@huawei.com> > Just a reminder to notify that this is a v2 fix to replace original > solution (which is in next branch) > https://patchwork.ozlabs.org/project/linux-mtd/patch/20221118090236.664244-3-chengzhihao1@huawei.com/. We cannot replace the patch in linux-next, the merge window is already open. A follow up fix is possible after 6.3-rc1 is done. So, next week. Thanks, //richard
在 2023/3/1 16:06, Richard Weinberger 写道: > We cannot replace the patch in linux-next, the merge window is already open. > A follow up fix is possible after 6.3-rc1 is done. So, next week. It's okay, thanks. I will send fix patch again after 6.3-rc1.
----- Ursprüngliche Mail ----- > Von: "chengzhihao1" <chengzhihao1@huawei.com> > An: "richard" <richard@nod.at> > CC: "Miquel Raynal" <miquel.raynal@bootlin.com>, "Sascha Hauer" <s.hauer@pengutronix.de>, "linux-mtd" > <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org> > Gesendet: Mittwoch, 1. März 2023 12:13:59 > Betreff: Re: [PATCH v2] ubifs: dirty_cow_znode: Fix memleak in error handling path > 在 2023/3/1 16:06, Richard Weinberger 写道: >> We cannot replace the patch in linux-next, the merge window is already open. >> A follow up fix is possible after 6.3-rc1 is done. So, next week. > > It's okay, thanks. I will send fix patch again after 6.3-rc1. You can send it right now. But I have to wait a bit. Thanks, //richard
diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c index 2df56bbc6865..6b7d95b65f4b 100644 --- a/fs/ubifs/tnc.c +++ b/fs/ubifs/tnc.c @@ -44,6 +44,33 @@ enum { NOT_ON_MEDIA = 3, }; +static void do_insert_old_idx(struct ubifs_info *c, + struct ubifs_old_idx *old_idx) +{ + struct ubifs_old_idx *o; + struct rb_node **p, *parent = NULL; + + p = &c->old_idx.rb_node; + while (*p) { + parent = *p; + o = rb_entry(parent, struct ubifs_old_idx, rb); + if (old_idx->lnum < o->lnum) + p = &(*p)->rb_left; + else if (old_idx->lnum > o->lnum) + p = &(*p)->rb_right; + else if (old_idx->offs < o->offs) + p = &(*p)->rb_left; + else if (old_idx->offs > o->offs) + p = &(*p)->rb_right; + else { + ubifs_err(c, "old idx added twice!"); + kfree(old_idx); + } + } + rb_link_node(&old_idx->rb, parent, p); + rb_insert_color(&old_idx->rb, &c->old_idx); +} + /** * insert_old_idx - record an index node obsoleted since the last commit start. * @c: UBIFS file-system description object @@ -69,35 +96,15 @@ enum { */ static int insert_old_idx(struct ubifs_info *c, int lnum, int offs) { - struct ubifs_old_idx *old_idx, *o; - struct rb_node **p, *parent = NULL; + struct ubifs_old_idx *old_idx; old_idx = kmalloc(sizeof(struct ubifs_old_idx), GFP_NOFS); if (unlikely(!old_idx)) return -ENOMEM; old_idx->lnum = lnum; old_idx->offs = offs; + do_insert_old_idx(c, old_idx); - p = &c->old_idx.rb_node; - while (*p) { - parent = *p; - o = rb_entry(parent, struct ubifs_old_idx, rb); - if (lnum < o->lnum) - p = &(*p)->rb_left; - else if (lnum > o->lnum) - p = &(*p)->rb_right; - else if (offs < o->offs) - p = &(*p)->rb_left; - else if (offs > o->offs) - p = &(*p)->rb_right; - else { - ubifs_err(c, "old idx added twice!"); - kfree(old_idx); - return 0; - } - } - rb_link_node(&old_idx->rb, parent, p); - rb_insert_color(&old_idx->rb, &c->old_idx); return 0; } @@ -199,23 +206,6 @@ static struct ubifs_znode *copy_znode(struct ubifs_info *c, __set_bit(DIRTY_ZNODE, &zn->flags); __clear_bit(COW_ZNODE, &zn->flags); - ubifs_assert(c, !ubifs_zn_obsolete(znode)); - __set_bit(OBSOLETE_ZNODE, &znode->flags); - - if (znode->level != 0) { - int i; - const int n = zn->child_cnt; - - /* The children now have new parent */ - for (i = 0; i < n; i++) { - struct ubifs_zbranch *zbr = &zn->zbranch[i]; - - if (zbr->znode) - zbr->znode->parent = zn; - } - } - - atomic_long_inc(&c->dirty_zn_cnt); return zn; } @@ -233,6 +223,42 @@ static int add_idx_dirt(struct ubifs_info *c, int lnum, int dirt) return ubifs_add_dirt(c, lnum, dirt); } +/** + * replace_znode - replace old znode with new znode. + * @c: UBIFS file-system description object + * @new_zn: new znode + * @old_zn: old znode + * @zbr: the branch of parent znode + * + * Replace old znode with new znode in TNC. + */ +static void replace_znode(struct ubifs_info *c, struct ubifs_znode *new_zn, + struct ubifs_znode *old_zn, struct ubifs_zbranch *zbr) +{ + ubifs_assert(c, !ubifs_zn_obsolete(old_zn)); + __set_bit(OBSOLETE_ZNODE, &old_zn->flags); + + if (old_zn->level != 0) { + int i; + const int n = new_zn->child_cnt; + + /* The children now have new parent */ + for (i = 0; i < n; i++) { + struct ubifs_zbranch *child = &new_zn->zbranch[i]; + + if (child->znode) + child->znode->parent = new_zn; + } + } + + zbr->znode = new_zn; + zbr->lnum = 0; + zbr->offs = 0; + zbr->len = 0; + + atomic_long_inc(&c->dirty_zn_cnt); +} + /** * dirty_cow_znode - ensure a znode is not being committed. * @c: UBIFS file-system description object @@ -265,21 +291,32 @@ static struct ubifs_znode *dirty_cow_znode(struct ubifs_info *c, return zn; if (zbr->len) { - err = insert_old_idx(c, zbr->lnum, zbr->offs); - if (unlikely(err)) - return ERR_PTR(err); + struct ubifs_old_idx *old_idx; + + old_idx = kmalloc(sizeof(struct ubifs_old_idx), GFP_NOFS); + if (unlikely(!old_idx)) { + err = -ENOMEM; + goto out; + } + old_idx->lnum = zbr->lnum; + old_idx->offs = zbr->offs; + err = add_idx_dirt(c, zbr->lnum, zbr->len); - } else - err = 0; + if (err) { + kfree(old_idx); + goto out; + } - zbr->znode = zn; - zbr->lnum = 0; - zbr->offs = 0; - zbr->len = 0; + do_insert_old_idx(c, old_idx); + } + + replace_znode(c, zn, znode, zbr); - if (unlikely(err)) - return ERR_PTR(err); return zn; + +out: + kfree(zn); + return ERR_PTR(err); } /**
Following process will cause a memleak for copied up znode: dirty_cow_znode zn = copy_znode(c, znode); err = insert_old_idx(c, zbr->lnum, zbr->offs); if (unlikely(err)) return ERR_PTR(err); // No one refers to zn. Fetch a reproducer in [Link]. Link: https://bugzilla.kernel.org/show_bug.cgi?id=216705 Fixes: 1e51764a3c2a ("UBIFS: add new flash file system") Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> --- v1: https://patchwork.ozlabs.org/project/linux-mtd/patch/20221118090236.664244-3-chengzhihao1@huawei.com/ v1->v2: In v1, if insert_old_idx() failed, old index neither exists in TNC nor in old-index tree. Which means that old index node could be overwritten in layout_leb_in_gaps(), then ubifs image will be corrupted in power-cut. In v2, copy_znode() is splitted into 2 parts: resouce allocation and znode replacement, insert_old_idx() is splitted in similar way, so resouce cleanup could be done in error handling path without corrupting metadata(mem & disk). It's okay that old index inserting is put behind of add_idx_dirt(), old index is used in layout_leb_in_gaps(), so the two processes do not depend on each other. fs/ubifs/tnc.c | 137 +++++++++++++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 50 deletions(-)