Re: [PATCH mm-unstable v4 10/10] mm/hugetlb: change hugetlb allocation functions to return a folio

[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,