From: Zhihao Cheng <chengzhihao1@huawei.com> To: Richard Weinberger <richard.weinberger@gmail.com> Cc: <linux-mtd@lists.infradead.org>, LKML <linux-kernel@vger.kernel.org>, Richard Weinberger <richard@nod.at>, "zhangyi (F)" <yi.zhang@huawei.com> Subject: Re: [PATCH 1/2] ubifs: xattr: Fix some potential memory leaks while iterating entries Date: Mon, 14 Sep 2020 11:24:55 +0800 [thread overview] Message-ID: <b82cd435-437d-e384-c95e-a7e031559c7e@huawei.com> (raw) In-Reply-To: <CAFLxGvxA9pw8D6Q8GbBD0SUP+EHhOsZmRMSPxrW4sq0gYi9N9Q@mail.gmail.com> 在 2020/9/14 3:08, Richard Weinberger 写道: > On Mon, Jun 1, 2020 at 11:11 AM Zhihao Cheng <chengzhihao1@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.
WARNING: multiple messages have this Message-ID (diff)
From: Zhihao Cheng <chengzhihao1@huawei.com> To: Richard Weinberger <richard.weinberger@gmail.com> Cc: Richard Weinberger <richard@nod.at>, linux-mtd@lists.infradead.org, LKML <linux-kernel@vger.kernel.org>, "zhangyi \(F\)" <yi.zhang@huawei.com> Subject: Re: [PATCH 1/2] ubifs: xattr: Fix some potential memory leaks while iterating entries Date: Mon, 14 Sep 2020 11:24:55 +0800 [thread overview] Message-ID: <b82cd435-437d-e384-c95e-a7e031559c7e@huawei.com> (raw) In-Reply-To: <CAFLxGvxA9pw8D6Q8GbBD0SUP+EHhOsZmRMSPxrW4sq0gYi9N9Q@mail.gmail.com> 在 2020/9/14 3:08, Richard Weinberger 写道: > On Mon, Jun 1, 2020 at 11:11 AM Zhihao Cheng <chengzhihao1@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. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-09-14 3:25 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-01 9:10 [PATCH 1/2] ubifs: xattr: Fix some potential memory leaks while iterating entries Zhihao Cheng 2020-06-01 9:10 ` Zhihao Cheng 2020-06-01 9:10 ` [PATCH 2/2] ubifs: dent: " Zhihao Cheng 2020-06-01 9:10 ` Zhihao Cheng 2020-09-13 19:08 ` [PATCH 1/2] ubifs: xattr: " Richard Weinberger 2020-09-13 19:08 ` Richard Weinberger 2020-09-14 3:24 ` Zhihao Cheng [this message] 2020-09-14 3:24 ` Zhihao Cheng 2020-09-17 20:47 ` Richard Weinberger 2020-09-17 20:47 ` Richard Weinberger
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b82cd435-437d-e384-c95e-a7e031559c7e@huawei.com \ --to=chengzhihao1@huawei.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=richard.weinberger@gmail.com \ --cc=richard@nod.at \ --cc=yi.zhang@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.