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 <[email protected]>
---
v1: https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/
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(-)
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);
}
/**
--
2.31.1
?? 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 <[email protected]>
> ---
> v1: https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/
> 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/[email protected]/.
> 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" <[email protected]>
> 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/[email protected]/.
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" <[email protected]>
> An: "richard" <[email protected]>
> CC: "Miquel Raynal" <[email protected]>, "Sascha Hauer" <[email protected]>, "linux-mtd"
> <[email protected]>, "linux-kernel" <[email protected]>
> 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