| Message ID | 1572339670-68694-1-git-send-email-chengzhihao1@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ubifs: do_kill_orphans: Fix a memory leak bug | expand |
On Tue, Oct 29, 2019 at 05:01:10PM +0800, Zhihao Cheng wrote: > If there are more than one valid snod on the sleb->nodes list, > do_kill_orphans will malloc ino more than once without releasing > previous ino's memory. Finally, it will trigger memory leak. > > Fixes: ee1438ce5dc4 ("ubifs: Check link count of inodes when...") > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > --- > fs/ubifs/orphan.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c > index 3b4b411..f211ed3 100644 > --- a/fs/ubifs/orphan.c > +++ b/fs/ubifs/orphan.c > @@ -673,9 +673,11 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb, > if (first) > first = 0; > > - ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS); > - if (!ino) > - return -ENOMEM; > + if (!ino) { > + ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS); > + if (!ino) > + return -ENOMEM; > + } This solves only part of the problem. There are several places in the loop that just return instead of jumping to out_free. These need to be fixed as well. I am not sure if it's worth it to allocate ino on demand. It would be more straight forward to allocate it once initially before the loop. Not sure though what Richard prefers. Regards, Sascha
On Tue, Oct 29, 2019 at 10:21 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > On Tue, Oct 29, 2019 at 05:01:10PM +0800, Zhihao Cheng wrote: > > If there are more than one valid snod on the sleb->nodes list, > > do_kill_orphans will malloc ino more than once without releasing > > previous ino's memory. Finally, it will trigger memory leak. > > > > Fixes: ee1438ce5dc4 ("ubifs: Check link count of inodes when...") > > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> > > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > > --- > > fs/ubifs/orphan.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c > > index 3b4b411..f211ed3 100644 > > --- a/fs/ubifs/orphan.c > > +++ b/fs/ubifs/orphan.c > > @@ -673,9 +673,11 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb, > > if (first) > > first = 0; > > > > - ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS); > > - if (!ino) > > - return -ENOMEM; > > + if (!ino) { > > + ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS); > > + if (!ino) > > + return -ENOMEM; > > + } > > This solves only part of the problem. There are several places in the > loop that just return instead of jumping to out_free. These need to be > fixed as well. > I am not sure if it's worth it to allocate ino on demand. It would be > more straight forward to allocate it once initially before the loop. > Not sure though what Richard prefers. Yeah, allocating it outside the loop once would be the best solution. I don't know why I did it in the loop. ;-\
diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c index 3b4b411..f211ed3 100644 --- a/fs/ubifs/orphan.c +++ b/fs/ubifs/orphan.c @@ -673,9 +673,11 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb, if (first) first = 0; - ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS); - if (!ino) - return -ENOMEM; + if (!ino) { + ino = kmalloc(UBIFS_MAX_INO_NODE_SZ, GFP_NOFS); + if (!ino) + return -ENOMEM; + } n = (le32_to_cpu(orph->ch.len) - UBIFS_ORPH_NODE_SZ) >> 3; for (i = 0; i < n; i++) {
If there are more than one valid snod on the sleb->nodes list, do_kill_orphans will malloc ino more than once without releasing previous ino's memory. Finally, it will trigger memory leak. Fixes: ee1438ce5dc4 ("ubifs: Check link count of inodes when...") Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> --- fs/ubifs/orphan.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)