[PATCH v2] hugetlbfs: fix protential null pointer dereference

[PATCH v2] hugetlbfs: fix protential null pointer dereference

[Yufen Yu]

This patch can avoid protential null pointer dereference for resv_map.

As Mike Kravetz say:
    Even if we can not hit this condition today, I still believe it
    would be a good idea to make this type of change.  It would
    prevent a possible NULL dereference in case the structure of code
    changes in the future.

Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Michal Hocko <mhocko@kernel.org>
Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 mm/hugetlb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 97b1e0290c66..fe74f94e5327 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4465,6 +4465,8 @@ int hugetlb_reserve_pages(struct inode *inode,
 	 */
 	if (!vma || vma->vm_flags & VM_MAYSHARE) {
 		resv_map = inode_resv_map(inode);
+		if (!resv_map)
+			return -EACCES;
 
 		chg = region_chg(resv_map, from, to);
 
-- 
2.16.2.dirty

Re: [PATCH v2] hugetlbfs: fix protential null pointer dereference

[Michal Hocko]

On Thu 11-04-19 11:53:18, Yufen Yu wrote:
> This patch can avoid protential null pointer dereference for resv_map.
> 
> As Mike Kravetz say:
>     Even if we can not hit this condition today, I still believe it
>     would be a good idea to make this type of change.  It would
>     prevent a possible NULL dereference in case the structure of code
>     changes in the future.

What kind of change would that be and wouldn't it require much more
changes?

In other words it is not really clear why is this an improvement. Random
checks for NULL that cannot happen tend to be more confusing long term
because people will simply blindly follow them and build a cargo cult
around.

> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
> ---
>  mm/hugetlb.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 97b1e0290c66..fe74f94e5327 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4465,6 +4465,8 @@ int hugetlb_reserve_pages(struct inode *inode,
>  	 */
>  	if (!vma || vma->vm_flags & VM_MAYSHARE) {
>  		resv_map = inode_resv_map(inode);
> +		if (!resv_map)
> +			return -EACCES;
>  
>  		chg = region_chg(resv_map, from, to);
>  
> -- 
> 2.16.2.dirty

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] hugetlbfs: fix protential null pointer dereference

[Mike Kravetz]

On 4/11/19 1:19 AM, Michal Hocko wrote:
> On Thu 11-04-19 11:53:18, Yufen Yu wrote:
>> This patch can avoid protential null pointer dereference for resv_map.
>>
>> As Mike Kravetz say:
>>     Even if we can not hit this condition today, I still believe it
>>     would be a good idea to make this type of change.  It would
>>     prevent a possible NULL dereference in case the structure of code
>>     changes in the future.
> 
> What kind of change would that be and wouldn't it require much more
> changes?
> 
> In other words it is not really clear why is this an improvement. Random
> checks for NULL that cannot happen tend to be more confusing long term
> because people will simply blindly follow them and build a cargo cult
> around.

Since that was my comment, I should reply.

You are correct in that it would require significant changes to hit this
issue.  I 'think' Yufen Yu came up with this patch by examining the hugetlbfs
code and noticing that this is the ONLY place where we do not check for
NULL.  Since I knew those other NULL checks were required, I was initially
concerned about this situation.  It took me some time and analysis to convince
myself that this was OK.  I don't want to make someone else repeat that.
Perhaps we should just comment this to avoid any confusion?

/*
 * resv_map can not be NULL here.  hugetlb_reserve_pages is only called from
 * two places:
 * 1) hugetlb_file_setup. In this case the inode is created immediately before
 *    the call with S_IFREG.  Hence a regular file so resv_map created.
 * 2) hugetlbfs_file_mmap called via do_mmap.  In do_mmap, there is the
 *    following check:
 *      if (!file->f_op->mmap)
 *              return -ENODEV;
 *    hugetlbfs_get_inode only assigns hugetlbfs_file_operations to S_IFREG
 *    inodes.  Hence, resv_map will not be NULL.
 */

Or, do you think that is too much?
Ideally, that comment should have been added as part of 58b6e5e8f1ad
("hugetlbfs: fix memory leak for resv_map") as it could cause one to wonder
if resv_map could be NULL.
-- 
Mike Kravetz

Re: [PATCH v2] hugetlbfs: fix protential null pointer dereference

[Michal Hocko]

On Thu 11-04-19 09:52:45, Mike Kravetz wrote:
> On 4/11/19 1:19 AM, Michal Hocko wrote:
> > On Thu 11-04-19 11:53:18, Yufen Yu wrote:
> >> This patch can avoid protential null pointer dereference for resv_map.
> >>
> >> As Mike Kravetz say:
> >>     Even if we can not hit this condition today, I still believe it
> >>     would be a good idea to make this type of change.  It would
> >>     prevent a possible NULL dereference in case the structure of code
> >>     changes in the future.
> > 
> > What kind of change would that be and wouldn't it require much more
> > changes?
> > 
> > In other words it is not really clear why is this an improvement. Random
> > checks for NULL that cannot happen tend to be more confusing long term
> > because people will simply blindly follow them and build a cargo cult
> > around.
> 
> Since that was my comment, I should reply.
> 
> You are correct in that it would require significant changes to hit this
> issue.  I 'think' Yufen Yu came up with this patch by examining the hugetlbfs
> code and noticing that this is the ONLY place where we do not check for
> NULL.  Since I knew those other NULL checks were required, I was initially
> concerned about this situation.  It took me some time and analysis to convince
> myself that this was OK.  I don't want to make someone else repeat that.
> Perhaps we should just comment this to avoid any confusion?
> 
> /*
>  * resv_map can not be NULL here.  hugetlb_reserve_pages is only called from
>  * two places:
>  * 1) hugetlb_file_setup. In this case the inode is created immediately before
>  *    the call with S_IFREG.  Hence a regular file so resv_map created.
>  * 2) hugetlbfs_file_mmap called via do_mmap.  In do_mmap, there is the
>  *    following check:
>  *      if (!file->f_op->mmap)
>  *              return -ENODEV;
>  *    hugetlbfs_get_inode only assigns hugetlbfs_file_operations to S_IFREG
>  *    inodes.  Hence, resv_map will not be NULL.
>  */
> 
> Or, do you think that is too much?
> Ideally, that comment should have been added as part of 58b6e5e8f1ad
> ("hugetlbfs: fix memory leak for resv_map") as it could cause one to wonder
> if resv_map could be NULL.

I would much rather explain a comment explaining _when_ inode_resv_map
might return NULL than add checks just to be sure.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] hugetlbfs: fix protential null pointer dereference

[Mike Kravetz]

On 4/11/19 11:22 AM, Michal Hocko wrote:
> On Thu 11-04-19 09:52:45, Mike Kravetz wrote:
>> Or, do you think that is too much?
>> Ideally, that comment should have been added as part of 58b6e5e8f1ad
>> ("hugetlbfs: fix memory leak for resv_map") as it could cause one to wonder
>> if resv_map could be NULL.
> 
> I would much rather explain a comment explaining _when_ inode_resv_map
> might return NULL than add checks just to be sure.

You are right.  That would make more sense.  It has been a while since I
looked into that code and unfortunately I did not save notes.  I'll do some
research to come up with an appropriate explanation/comment.

-- 
Mike Kravetz

Re: [PATCH v2] hugetlbfs: fix protential null pointer dereference

[Michal Hocko]

On Thu 11-04-19 11:40:02, Mike Kravetz wrote:
> On 4/11/19 11:22 AM, Michal Hocko wrote:
> > On Thu 11-04-19 09:52:45, Mike Kravetz wrote:
> >> Or, do you think that is too much?
> >> Ideally, that comment should have been added as part of 58b6e5e8f1ad
> >> ("hugetlbfs: fix memory leak for resv_map") as it could cause one to wonder
> >> if resv_map could be NULL.
> > 
> > I would much rather explain a comment explaining _when_ inode_resv_map
> > might return NULL than add checks just to be sure.
> 
> You are right.  That would make more sense.  It has been a while since I
> looked into that code and unfortunately I did not save notes.  I'll do some
> research to come up with an appropriate explanation/comment.

Thanks a lot! This is highly appreciated.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] hugetlbfs: fix protential null pointer dereference

[William Kucharski]

> On Apr 11, 2019, at 12:40 PM, Mike Kravetz <mike.kravetz@oracle.com> wrote:
> 
> You are right.  That would make more sense.  It has been a while since I
> looked into that code and unfortunately I did not save notes.  I'll do some
> research to come up with an appropriate explanation/comment.

I also like the idea of a comment rather than code here.

Zero run time impact and more importantly it instantly explains to everyone
why this case is different rather than just adding the check for the sake of
uniformity.