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
