On Sat, Jul 10, 2021 at 8:25 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote: > > When removing a hugetlb page from the pool the ref count is set to > one (as the free page has no ref count) and compound page destructor > is set to NULL_COMPOUND_DTOR. Since a subsequent call to free the > hugetlb page will call __free_pages for non-gigantic pages and > free_gigantic_page for gigantic pages the destructor is not used. > > However, consider the following race with code taking a speculative > reference on the page: > > Thread 0 Thread 1 > -------- -------- > remove_hugetlb_page > set_page_refcounted(page); > set_compound_page_dtor(page, > NULL_COMPOUND_DTOR); > get_page_unless_zero(page) > __update_and_free_page > __free_pages(page, > huge_page_order(h)); > > /* Note that __free_pages() will simply drop > the reference to the page. */ > > put_page(page) > __put_compound_page() > destroy_compound_page > NULL_COMPOUND_DTOR > BUG: kernel NULL pointer > dereference, address: > 0000000000000000 > > To address this race, set the dtor to the normal compound page dtor > for non-gigantic pages. The dtor for gigantic pages does not matter > as gigantic pages are changed from a compound page to 'just a group of > pages' before freeing. Hence, the destructor is not used. > > Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx> > --- > mm/hugetlb.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 3132c7395743..fa8ec2072949 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -1370,8 +1370,28 @@ static void remove_hugetlb_page(struct hstate *h, struct page *page, > h->surplus_huge_pages_node[nid]--; > } > > + /* > + * Very subtle > + * > + * For non-gigantic pages set the destructor to the normal compound > + * page dtor. This is needed in case someone takes an additional > + * temporary ref to the page, and freeing is delayed until they drop > + * their reference. > + * > + * For gigantic pages set the destructor to the null dtor. This > + * destructor will never be called. Before freeing the gigantic > + * page destroy_compound_gigantic_page will turn the compound page > + * into a simple group of pages. After this the destructor does not > + * apply. > + * > + * This handles the case where more than one ref is held when and > + * after update_and_free_page is called. > + */ > set_page_refcounted(page); > - set_compound_page_dtor(page, NULL_COMPOUND_DTOR); > + if (hstate_is_gigantic(h)) > + set_compound_page_dtor(page, NULL_COMPOUND_DTOR); > + else > + set_compound_page_dtor(page, COMPOUND_PAGE_DTOR); Hi Mike, The race is really subtle. But we also should remove the WARN from free_contig_range, right? Because the refcount of the head page of the gigantic page can be greater than one, but free_contig_range has the following warning. WARN(count != 0, "%lu pages are still in use!\n", count); Thanks. > > h->nr_huge_pages--; > h->nr_huge_pages_node[nid]--; > -- > 2.31.1 >
