Re: [PATCH] udf: prevent memory leak in udf_new_inode
On Thu, Sep 26, 2019 at 10:00:31AM +0200, Jan Kara wrote: > On Wed 25-09-19 23:24:08, Al Viro wrote: > > 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 > > > --- > > > 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, ); > > > 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. > 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? > > Exactly. udf_evict_inode() is responsible for freeing iinfo->i_ext.i_data > so the patch would result in double free. > > Honza Thanks for clarification. > -- > Jan Kara > SUSE Labs, CR
Re: [PATCH] udf: prevent memory leak in udf_new_inode
On Wed 25-09-19 23:24:08, Al Viro wrote: > 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 > > --- > > 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, ); > > 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. 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? Exactly. udf_evict_inode() is responsible for freeing iinfo->i_ext.i_data so the patch would result in double free. Honza -- Jan Kara SUSE Labs, CR
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 > --- > 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, ); > 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. 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.
[PATCH] udf: prevent memory leak in udf_new_inode
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. Signed-off-by: Navid Emamdoost --- 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, ); if (err) { + kfree(iinfo->i_ext.i_data); iput(inode); return ERR_PTR(err); } @@ -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); -- 2.17.1