ubifs: Fix memleak when insert_old_idx() failed

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].

Function copy_znode() is split into 2 parts: resource allocation
and znode replacement, insert_old_idx() is split in similar way,
so resource 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.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216705
Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Cc: stable@vger.kernel.org
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Signed-off-by: Richard Weinberger <richard@nod.at>

diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index 2df56bb..6b7d95b 100644 (file)
--- 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;
 }
 
@@ -234,6 +224,42 @@ static int add_idx_dirt(struct ubifs_info *c, int lnum, int 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
  * @zbr: branch of znode to check
@@ -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);
 }
 
 /**