Messages in this thread | | | Date | Mon, 21 Nov 2022 07:46:03 -0800 | Subject | Re: [PATCH mm-unstable v4 10/10] mm/hugetlb: change hugetlb allocation functions to return a folio | From | Sidhartha Kumar <> |
| |
On 11/20/22 6:31 PM, John Hubbard wrote: > On 11/18/22 14:20, Sidhartha Kumar wrote: > ... >> @@ -1950,7 +1949,7 @@ pgoff_t hugetlb_basepage_index(struct page *page) >> return (index << compound_order(page_head)) + compound_idx; >> } >> -static struct page *alloc_buddy_huge_page(struct hstate *h, >> +static struct folio *alloc_buddy_hugetlb_folio(struct hstate *h, >> gfp_t gfp_mask, int nid, nodemask_t *nmask, >> nodemask_t *node_alloc_noretry) >> { >> @@ -2009,7 +2008,7 @@ static struct page >> *alloc_buddy_huge_page(struct hstate *h, >> if (node_alloc_noretry && !page && alloc_try_hard) >> node_set(nid, *node_alloc_noretry); >> - return page; >> + return page_folio(page); > > 1. This causes a NULL pointer crash when the user requests too many > hugetlb > pages (you can probably guess how I know this, haha), for example: > > echo 50000 > /proc/sys/vm/nr_hugepages > > ...because page_folio() doesn't have a NULL check in there. You will want > to do something like this, on top of this current patch: > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 629bb044f063..ffb0f052bbff 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -1987,11 +1987,6 @@ static struct folio > *alloc_buddy_hugetlb_folio(struct hstate *h, > page = NULL; > } > > - if (page) > - __count_vm_event(HTLB_BUDDY_PGALLOC); > - else > - __count_vm_event(HTLB_BUDDY_PGALLOC_FAIL); > - > /* > * If we did not specify __GFP_RETRY_MAYFAIL, but still got a > page this > * indicates an overall state change. Clear bit so that we > resume > @@ -2008,6 +2003,12 @@ static struct folio > *alloc_buddy_hugetlb_folio(struct hstate *h, > if (node_alloc_noretry && !page && alloc_try_hard) > node_set(nid, *node_alloc_noretry); > > + if (!page) { > + __count_vm_event(HTLB_BUDDY_PGALLOC_FAIL); > + return NULL; > + } > + > + __count_vm_event(HTLB_BUDDY_PGALLOC); > return page_folio(page); > } > > > 2. And also, the tests should probably be augmented to run this simple > (but easy to overlook) test. > Sure, I can look into adding this type of test to Linux Test Project.
> 3. And finally, the basic method of replacing page with page_folio(page) > is not sufficient, as you can see here. So I'd suggest taking a look > through your series to see if you are checking for NULL first, before > calling page_folio(page). > I will go through my series again to check for possible NULL dereferences.
Thanks for taking a look.
Sidhartha Kumar
> > thanks,
| |