Messages in this thread | | | Date | Wed, 25 Sep 2019 23:24:08 +0100 | From | Al Viro <> | Subject | Re: [PATCH] udf: prevent memory leak in udf_new_inode |
| |
On Wed, Sep 25, 2019 at 04:39:03PM -0500, Navid Emamdoost wrote: > In udf_new_inode if either udf_new_block or insert_inode_locked fials > the allocated memory for iinfo->i_ext.i_data should be released.
"... because of such-and-such reasons" part appears to be missing. Why should it be released there?
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > fs/udf/ialloc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c > index 0adb40718a5d..b8ab3acab6b6 100644 > --- a/fs/udf/ialloc.c > +++ b/fs/udf/ialloc.c > @@ -86,6 +86,7 @@ struct inode *udf_new_inode(struct inode *dir, umode_t mode) > dinfo->i_location.partitionReferenceNum, > start, &err); > if (err) { > + kfree(iinfo->i_ext.i_data); > iput(inode); > return ERR_PTR(err); > }
Have you tested that? Because it has all earmarks of double-free; normal eviction pathway ought to free the damn thing. <greps around a bit>
Mind explaining what's to stop ->evict_inode (== udf_evict_inode) from hitting kfree(iinfo->i_ext.i_data); considering that this call of kfree() appears to be unconditional there?
> @@ -130,6 +131,7 @@ struct inode *udf_new_inode(struct inode *dir, umode_t mode) > inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); > iinfo->i_crtime = inode->i_mtime; > if (unlikely(insert_inode_locked(inode) < 0)) { > + kfree(iinfo->i_ext.i_data); > make_bad_inode(inode); > iput(inode); > return ERR_PTR(-EIO);
And the same here.
| |