Re: [PATCH v2 5.15 2/2] mm/hugetlb: fix kernel NULL pointer dereference when replacing free hugetlb folios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm/hugetlb: compound_head() evaluated outside hugetlb_lock in
  replace_free_hugepage_folios()
--

commit de1b340d0acd3398ffa967e9c36bcd2099c3b383
Author: Ge Yang <yangge1116@126.com>

mm/hugetlb: fix kernel NULL pointer dereference when replacing free hugetlb folios

This commit acquires hugetlb_lock around PageHuge() and page_hstate() in
replace_free_hugepage_folios() to prevent racing with concurrent hugepage
freeing.

Link: https://lkml.kernel.org/r/1747884137-26685-1-git-send-email-yangge1116@126.com

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 4fb98320f9df3..3d7e8826e4525 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2756,12 +2756,20 @@ int replace_free_hugepage_folios(unsigned long start_pfn, unsigned long end_pfn)
>  
>  	while (start_pfn < end_pfn) {
>  		page = compound_head(pfn_to_page(start_pfn));
> +
> +		/*
> +		 * The page might have been dissolved from under our feet, so make sure
> +		 * to carefully check the state under the lock.
> +		 */
> +		spin_lock_irq(&hugetlb_lock);
>  		if (PageHuge(page)) {
>  			h = page_hstate(page);
>  		} else {
> +			spin_unlock_irq(&hugetlb_lock);
>  			start_pfn++;
>  			continue;
>  		}
> +		spin_unlock_irq(&hugetlb_lock);

[Severity: Medium]
Can page_hstate(page) still return NULL if pfn_to_page(start_pfn) becomes a
tail page of a newly prepared hugepage before hugetlb_lock is acquired?

CPU0                                    CPU1
replace_free_hugepage_folios()
  page = compound_head(pfn_to_page(start_pfn))
                                        alloc_gigantic_page() / prep_new_huge_page()
                                          prep_compound_gigantic_page()
                                            /* page becomes a tail page */
                                          spin_lock_irq(&hugetlb_lock)
                                          __prep_new_huge_page()
                                            set_compound_page_dtor(HUGETLB_PAGE_DTOR)
                                          spin_unlock_irq(&hugetlb_lock)
  spin_lock_irq(&hugetlb_lock)
  if (PageHuge(page))
    h = page_hstate(page)  <-- returns NULL
  spin_unlock_irq(&hugetlb_lock)
  alloc_and_dissolve_huge_page(h, page, ...)
    htlb_alloc_mask(h)  <-- NULL pointer dereference

In upstream 113ed54ad276 ("mm/hugetlb: fix kernel NULL pointer dereference
when replacing free hugetlb folios"), pfn_folio(start_pfn) is checked with
folio_test_hugetlb(), which returns false for tail pages.  In 5.15,
PageHuge(page) resolves compound_head(page) internally on its local parameter
and returns true for tail pages, while page_hstate(page) calls
page_size(page) -> compound_order(page), which returns 0 when !PageHead(page)
and causes size_to_hstate(PAGE_SIZE) to return NULL.

Re-evaluating page = compound_head(page) under hugetlb_lock after
PageHuge(page) succeeds, as isolate_or_dissolve_huge_page() and
dissolve_free_huge_page() do, avoids passing a tail page to page_hstate() and
alloc_and_dissolve_huge_page().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106220445.2859944-1-mkbund@amazon.com?part=2