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

[Sidhartha Kumar <sidhartha.kumar@oracle.com>]

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,