[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <03ca2e41-28d3-c546-4347-5dee7b7dca9b@oracle.com>
Date: Mon, 21 Nov 2022 07:46:03 -0800
From: Sidhartha Kumar <sidhartha.kumar@...cle.com>
To: John Hubbard <jhubbard@...dia.com>, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Cc: akpm@...ux-foundation.org, songmuchun@...edance.com,
mike.kravetz@...cle.com, willy@...radead.org,
almasrymina@...gle.com, linmiaohe@...wei.com, hughd@...gle.com
Subject: Re: [PATCH mm-unstable v4 10/10] mm/hugetlb: change hugetlb
allocation functions to return a folio
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,
Powered by blists - more mailing lists