Re: [PATCH 1/2] ubifs: xattr: Fix some potential memory leaks while iterating entries

[Zhihao Cheng]

在 2020/9/14 3:08, Richard Weinberger 写道:
On Mon, Jun 1, 2020 at 11:11 AM Zhihao Cheng <chengzhih...@huawei.com> wrote:


I agree that this needs fixing. Did you also look into getting rid of pxent?
UBIFS uses the pxent pattern over and over and the same error got copy pasted
a lot. :-(

I thought about it. I'm not sure whether it is good to drop 'pxent'. 
Maybe you mean doing changes looks like following(Takes 
ubifs_jnl_write_inode() for example):

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 4a5b06f8d812..fcd5ac047b34 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -879,13 +879,19 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
                union ubifs_key key;
                struct fscrypt_name nm = {0};
                struct inode *xino;
-               struct ubifs_dent_node *xent, *pxent = NULL;
+               struct ubifs_dent_node *xent;

                if (ui->xattr_cnt >= ubifs_xattr_max_cnt(c)) {
                        ubifs_err(c, "Cannot delete inode, it has too much 
xattrs!");
                        goto out_release;
                }

+               fname_name(&nm) = kmalloc(UBIFS_MAX_NLEN, GFP_NOFS);
+               if (!fname_name(&nm)) {
+                       err = -ENOMEM;
+                       goto out_release;
+               }
+
                lowest_xent_key(c, &key, inode->i_ino);
                while (1) {
                        xent = ubifs_tnc_next_ent(c, &key, &nm);
@@ -894,11 +900,12 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
                                if (err == -ENOENT)
                                        break;

+                               kfree(fname_name(&nm));
                                goto out_release;
                        }

-                       fname_name(&nm) = xent->name;
                        fname_len(&nm) = le16_to_cpu(xent->nlen);
+                       strncpy(fname_name(&nm), xent->name, fname_len(&nm));

                        xino = ubifs_iget(c->vfs_sb, le64_to_cpu(xent->inum));
                        if (IS_ERR(xino)) {
@@ -907,6 +914,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
                                          xent->name, err);
                                ubifs_ro_mode(c, err);
                                kfree(xent);
+                               kfree(fname_name(&nm));
                                goto out_release;
                        }
                        ubifs_assert(c, ubifs_inode(xino)->xattr);
@@ -916,11 +924,10 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
                        ino = (void *)ino + UBIFS_INO_NODE_SZ;
                        iput(xino);

-                       kfree(pxent);
-                       pxent = xent;
                        key_read(c, &xent->key, &key);
+                       kfree(xent);
                }
-               kfree(pxent);
+               kfree(fname_name(&nm));
        }

        pack_inode(c, ino, inode, 1);

The kill_xattrs process is more intuitive without the pxent. However, 
the release process for the memory (stores xent->name) is similar to 
'pxent'. If you think it's better than v1, I will send v2.