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

[Michal Hocko <mhocko@kernel.org>]

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