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
