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

[yuyufen <yuyufen@huawei.com>]

On 2019/4/11 2:56, Mike Kravetz wrote:
> On 4/9/19 9:20 PM, yuyufen wrote:
>> Hi, Mike
>>
>> On 2019/4/10 11:38, Mike Kravetz wrote:
>>> On 4/9/19 7:50 PM, Yufen Yu wrote:
>>>> After commit 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map"),
>>>> i_mapping->private_data will be NULL for mode that is not regular and link.
>>>> Then, it might cause NULL pointer derefernce in hugetlb_reserve_pages()
>>>> when do_mmap. We can avoid protential null pointer dereference by
>>>> judging whether it have been allocated.
>>>>
>>>> Fixes: 58b6e5e8f1ad ("hugetlbfs: fix memory leak for resv_map")
>>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>>> Cc: Michal Hocko <mhocko@kernel.org>
>>>> Signed-off-by: Yufen Yu <yuyufen@huawei.com>
>>> Thanks for catching this.  I mistakenly thought all the code was checking
>>> for NULL resv_map.  That certainly is one (and only) place where it is not
>>> checked.  Have you verified that this is possible?  Should be pretty easy
>>> to do.  If you have not, I can try to verify tomorrow.
>> I honestly say that I don't have verified.
> I do not believe it is possible to hit this condition in the existing code.
> Why?  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;
>     In the hugetlbfs inode creation code (hugetlbfs_get_inode), note that
>     inode->i_fop = &hugetlbfs_file_operations (containing hugetlbfs_file_mmap)
>     is only set for inodes of type S_IFREG.  And, resv_map are created
>     for these.  So, mmap will not call hugetlbfs_file_mmap for non-S_IFREG
>     hugetlbfs inode.  Instead, it will return ENODEV.
>
> 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.  However,
> unless I am mistaken this is not needed as an urgent fix.

Thanks for so much detailed explanation. I will resend v2 including 
these suggestion.