Re: [PATCH] hugetlbfs: move resv_map to hugetlbfs_inode_info

[Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>]

On Fri, Apr 12, 2019 at 04:40:01PM -0700, Mike Kravetz wrote:
> On 4/11/19 9:02 PM, Yufen Yu wrote:
> > Commit 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map")
> ...
> > However, for inode mode that is 'S_ISBLK', hugetlbfs_evict_inode() may
> > free or modify i_mapping->private_data that is owned by bdev inode,
> > which is not expected!
> ...
> > We fix the problem by moving resv_map to hugetlbfs_inode_info. It may
> > be more reasonable.
> 
> Your patches force me to consider these potential issues.  Thank you!
> 
> The root of all these problems (including the original leak) is that the
> open of a block special inode will result in bd_acquire() overwriting the
> value of inode->i_mapping.  Since hugetlbfs inodes normally contain a
> resv_map at inode->i_mapping->private_data, a memory leak occurs if we do
> not free the initially allocated resv_map.  In addition, when the
> inode is evicted/destroyed inode->i_mapping may point to an address space
> not associated with the hugetlbfs inode.  If code assumes inode->i_mapping
> points to hugetlbfs inode address space at evict time, there may be bad
> data references or worse.

Let me ask a kind of elementary question: is there any good reason/purpose
to create and use block special files on hugetlbfs?  I never heard about
such usecases.  I guess that the conflict of the usage of ->i_mapping is
discovered recently and that's because block special files on hugetlbfs are
just not considered until recently or well defined.  So I think that we might
be better to begin with defining it first.

I tried the procedure described in commit 58b6e5e8f1a ("hugetlbfs: fix
memory leak for resv_map") and I failed to open the block special file with
ENXIO. So I'm not clearly sure what to solve.
I tried a similar test on tmpfs (good reference for hugetlbfs) and that also
failed on opening block file on it (but with EACCESS), so simply fixing it
similarly could be an option if there's no reasonable usecase and we just
want to fix the memory leak. 

> 
> This specific part of the patch made me think,
> 
> > @@ -497,12 +497,15 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
> >  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);
> >  }
> 
> If inode->i_mapping may not be associated with the hugetlbfs inode, then
> remove_inode_hugepages() will also have problems.  It will want to operate
> on the address space associated with the inode.  So, there are more issues
> than just the resv_map.  When I looked at the first few lines of
> remove_inode_hugepages(), I was surprised to see:
> 
> 	struct address_space *mapping = &inode->i_data;
> 
> So remove_inode_hugepages is explicitly using the original address space
> that is embedded in the inode.  As a result, it is not impacted by changes
> to inode->i_mapping.  Using git history I was unable to determine why
> remove_inode_hugepages is the only place in hugetlbfs code doing this.
> 
> With this in mind, a simple change like the following will fix the original
> leak issue as well as the potential issues mentioned in this patch.
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 53ea3cef526e..9f0719bad46f 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -511,6 +511,11 @@ static void hugetlbfs_evict_inode(struct inode *inode)
>  {
>  	struct resv_map *resv_map;
>  
> +	/*
> +	 * Make sure we are operating on original hugetlbfs address space.
> +	 */
> +	inode->i_mapping = &inode->i_data;
> +
>  	remove_inode_hugepages(inode, 0, LLONG_MAX);
>  	resv_map = (struct resv_map *)inode->i_mapping->private_data;
>  	/* root inode doesn't have the resv_map, so we should check it */
> 
> 
> I don't know why hugetlbfs code would ever want to operate on any address
> space but the one embedded within the inode.  However, my uderstanding of
> the vfs layer is somewhat limited.  I'm wondering if the hugetlbfs code
> (helper routines mostly) should perhaps use &inode->i_data instead of
> inode->i_mapping.  Does it ever make sense for hugetlbfs code to operate
> on inode->i_mapping if inode->i_mapping != &inode->i_data ?

I'm not a fs expert, but &inode->i_data seems safer pointer to reach to
the struct address_space because it's embedded in inode and always exists.

Thanks,
Naoya Horiguchi