| Message ID | 20190411035318.32976-1-yuyufen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hugetlbfs: fix protential null pointer dereference | expand |
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
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.
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.
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.
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.
> 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.
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);
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(+)