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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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,




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux