[PATCH] hugetlbfs: fix memory leak for resv_map

[PATCH] hugetlbfs: fix memory leak for resv_map

[Yufen Yu]

When .mknod create a block device file in hugetlbfs, it will
allocate an inode, and kmalloc a 'struct resv_map' in resv_map_alloc().
For now, inode->i_mapping->private_data is used to point the resv_map.
However, when open the device, bd_acquire() will set i_mapping as
bd_inode->imapping, result in resv_map memory leak.

We fix the leak by adding a new entry resv_map in hugetlbfs_inode_info.
It can store resv_map pointer.

Programs to reproduce:
	mount -t hugetlbfs nodev hugetlbfs
	mknod hugetlbfs/dev b 0 0
	exec 30<> hugetlbfs/dev
	umount hugetlbfs/

Fixes: 9119a41e90 ("mm, hugetlb: unify region structure handling")
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 fs/hugetlbfs/inode.c    | 12 +++++++++---
 include/linux/hugetlb.h |  1 +
 mm/hugetlb.c            |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index a7fa037b876b..7f332443c5da 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -484,11 +484,15 @@ static void hugetlbfs_evict_inode(struct inode *inode)
 {
 	struct resv_map *resv_map;
 
+	struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
+
 	remove_inode_hugepages(inode, 0, LLONG_MAX);
-	resv_map = (struct resv_map *)inode->i_mapping->private_data;
+	resv_map = info->resv_map;
 	/* root inode doesn't have the resv_map, so we should check it */
-	if (resv_map)
+	if (resv_map) {
 		resv_map_release(&resv_map->refs);
+		info->resv_map = NULL;
+	}
 	clear_inode(inode);
 }
 
@@ -757,7 +761,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 				&hugetlbfs_i_mmap_rwsem_key);
 		inode->i_mapping->a_ops = &hugetlbfs_aops;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
-		inode->i_mapping->private_data = resv_map;
+		info->resv_map = resv_map;
 		info->seals = F_SEAL_SEAL;
 		switch (mode & S_IFMT) {
 		default:
@@ -1025,6 +1029,7 @@ static struct inode *hugetlbfs_alloc_inode(struct super_block *sb)
 	 * private inode.  This simplifies hugetlbfs_destroy_inode.
 	 */
 	mpol_shared_policy_init(&p->policy, NULL);
+	p->resv_map = NULL;
 
 	return &p->vfs_inode;
 }
@@ -1039,6 +1044,7 @@ static void hugetlbfs_destroy_inode(struct inode *inode)
 {
 	hugetlbfs_inc_free_inodes(HUGETLBFS_SB(inode->i_sb));
 	mpol_free_shared_policy(&HUGETLBFS_I(inode)->policy);
+	HUGETLBFS_I(inode)->resv_map = NULL;
 	call_rcu(&inode->i_rcu, hugetlbfs_i_callback);
 }
 
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 087fd5f48c91..eafcf14056bc 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -291,6 +291,7 @@ struct hugetlbfs_inode_info {
 	struct shared_policy policy;
 	struct inode vfs_inode;
 	unsigned int seals;
+	struct resv_map *resv_map;
 };
 
 static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 8dfdffc34a99..763164b15f8f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -739,7 +739,7 @@ void resv_map_release(struct kref *ref)
 
 static inline struct resv_map *inode_resv_map(struct inode *inode)
 {
-	return inode->i_mapping->private_data;
+	return HUGETLBFS_I(inode)->resv_map;
 }
 
 static struct resv_map *vma_resv_map(struct vm_area_struct *vma)
-- 
2.16.2.dirty

Re: [PATCH] hugetlbfs: fix memory leak for resv_map

[Mike Kravetz]

Thank you for finding this issue.

On 3/2/19 2:47 AM, Yufen Yu wrote:
> When .mknod create a block device file in hugetlbfs, it will
> allocate an inode, and kmalloc a 'struct resv_map' in resv_map_alloc().
> For now, inode->i_mapping->private_data is used to point the resv_map.
> However, when open the device, bd_acquire() will set i_mapping as
> bd_inode->imapping, result in resv_map memory leak.

We are certainly leaking the resv_map.

> We fix the leak by adding a new entry resv_map in hugetlbfs_inode_info.
> It can store resv_map pointer.

This approach preserves the way the existing code always allocates a
resv_map at inode allocation time.  However, it does add an extra word
to every hugetlbfs inode.  My first thought was, why not special case
the block/char inode creation to not allocate a resv_map?  After all,
it is not used in this case.  In fact, we only need/use the resv_map
when mmap'ing a regular file.  It is a waste to allocate the structure
in all other cases.

It seems like we should be able to wait until a call to hugetlb_reserve_pages()
to allocate the inode specific resv_map in much the same way we do for
private mappings.  We could then remove the resv_map allocation at inode
creation time.  Of course, we would still need the code to free the structure
when the inode is destroyed.

I have not looked too closely at this approach, and there may be some
unknown issues.  However, it would address the leak you discovered and
would result in less memory used for hugetlbfs inodes that are never
mmap'ed.

Any thoughts on this approach?

I know it is beyond the scope of your patch.  If you do not want to try this,
I can code up something in a couple days.
-- 
Mike Kravetz

Re: [PATCH] hugetlbfs: fix memory leak for resv_map

[yuyufen]

Hi, Mike


On 2019/3/5 2:29, Mike Kravetz wrote:
> Thank you for finding this issue.
>
> On 3/2/19 2:47 AM, Yufen Yu wrote:
>> When .mknod create a block device file in hugetlbfs, it will
>> allocate an inode, and kmalloc a 'struct resv_map' in resv_map_alloc().
>> For now, inode->i_mapping->private_data is used to point the resv_map.
>> However, when open the device, bd_acquire() will set i_mapping as
>> bd_inode->imapping, result in resv_map memory leak.
> We are certainly leaking the resv_map.
>
>> We fix the leak by adding a new entry resv_map in hugetlbfs_inode_info.
>> It can store resv_map pointer.
> This approach preserves the way the existing code always allocates a
> resv_map at inode allocation time.  However, it does add an extra word
> to every hugetlbfs inode.  My first thought was, why not special case
> the block/char inode creation to not allocate a resv_map?  After all,
> it is not used in this case.  In fact, we only need/use the resv_map
> when mmap'ing a regular file.  It is a waste to allocate the structure
> in all other cases.
>
> It seems like we should be able to wait until a call to hugetlb_reserve_pages()
> to allocate the inode specific resv_map in much the same way we do for
> private mappings.  We could then remove the resv_map allocation at inode
> creation time.  Of course, we would still need the code to free the structure
> when the inode is destroyed.
>
> I have not looked too closely at this approach, and there may be some
> unknown issues.  However, it would address the leak you discovered and
> would result in less memory used for hugetlbfs inodes that are never
> mmap'ed.
>
> Any thoughts on this approach?
>
> I know it is beyond the scope of your patch.  If you do not want to try this,
> I can code up something in a couple days.

Thanks for your suggestion. I agree with you. It will be a better solution.
I don't understand hugetlbfs deeply, but I want to try my best to solve 
this problem.

Yufen
Thanks.

[PATCH] hugetlbfs: fix memory leak for resv_map

[Mike Kravetz]

When mknod is used to create a block special file in hugetlbfs, it will
allocate an inode and kmalloc a 'struct resv_map' via resv_map_alloc().
inode->i_mapping->private_data will point the newly allocated resv_map.
However, when the device special file is opened bd_acquire() will
set i_mapping as bd_inode->imapping.  Thus the pointer to the allocated
resv_map is lost and the structure is leaked.

Programs to reproduce:
        mount -t hugetlbfs nodev hugetlbfs
        mknod hugetlbfs/dev b 0 0
        exec 30<> hugetlbfs/dev
        umount hugetlbfs/

resv_map structures are only needed for inodes which can have associated
page allocations.  To fix the leak, only allocate resv_map for those
inodes which could possibly be associated with page allocations.

Reported-by: Yufen Yu <yuyufen@huawei.com>
Suggested-by: Yufen Yu <yuyufen@huawei.com>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 6189ba80b57b..f76e44d1aa54 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -752,11 +752,17 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 					umode_t mode, dev_t dev)
 {
 	struct inode *inode;
-	struct resv_map *resv_map;
+	struct resv_map *resv_map = NULL;
 
-	resv_map = resv_map_alloc();
-	if (!resv_map)
-		return NULL;
+	/*
+	 * Reserve maps are only needed for inodes that can have associated
+	 * page allocations.
+	 */
+	if (S_ISREG(mode) || S_ISLNK(mode)) {
+		resv_map = resv_map_alloc();
+		if (!resv_map)
+			return NULL;
+	}
 
 	inode = new_inode(sb);
 	if (inode) {
@@ -791,8 +797,10 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 			break;
 		}
 		lockdep_annotate_inode_mutex_key(inode);
-	} else
-		kref_put(&resv_map->refs, resv_map_release);
+	} else {
+		if (resv_map)
+			kref_put(&resv_map->refs, resv_map_release);
+	}
 
 	return inode;
 }
-- 
2.20.1