Re: [PATCH 1/3] mm: hugetlbfs: fix hugetlbfs optimization

[Andrew Morton]

On Fri, 15 Nov 2013 18:47:46 +0100 Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote:

> The patch from commit 7cb2ef56e6a8b7b368b2e883a0a47d02fed66911 can
> cause dereference of a dangling pointer if split_huge_page runs during
> PageHuge() if there are updates to the tail_page->private field.
> 
> Also it is repeating compound_head twice for hugetlbfs and it is
> running compound_head+compound_trans_head for THP when a single one is
> needed in both cases.
> 
> The new code within the PageSlab() check doesn't need to verify that
> the THP page size is never bigger than the smallest hugetlbfs page
> size, to avoid memory corruption.
> 
> A longstanding theoretical race condition was found while fixing the
> above (see the change right after the skip_unlock label, that is
> relevant for the compound_lock path too).
> 
> By re-establishing the _mapcount tail refcounting for all compound
> pages, this also fixes the below problem:
> 
> echo 0 >/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
> 
> ...
>
> +/*
> + * PageHeadHuge() only returns true for hugetlbfs head page, but not for
> + * normal or transparent huge pages.
> + */
> +int PageHeadHuge(struct page *page_head)
> +{
> +	compound_page_dtor *dtor;
> +
> +	if (!PageHead(page_head))
> +		return 0;
> +
> +	dtor = get_compound_page_dtor(page_head);
> +
> +	return dtor == free_huge_page;
> +}
> +EXPORT_SYMBOL_GPL(PageHeadHuge);

This is all rather verbose.  How about we do this?

--- a/mm/hugetlb.c~mm-hugetlbc-simplify-pageheadhuge-and-pagehuge
+++ a/mm/hugetlb.c
@@ -690,15 +690,11 @@ static void prep_compound_gigantic_page(
  */
 int PageHuge(struct page *page)
 {
-	compound_page_dtor *dtor;
-
 	if (!PageCompound(page))
 		return 0;
 
 	page = compound_head(page);
-	dtor = get_compound_page_dtor(page);
-
-	return dtor == free_huge_page;
+	return get_compound_page_dtor(page) == free_huge_page;
 }
 EXPORT_SYMBOL_GPL(PageHuge);
 
@@ -708,14 +704,10 @@ EXPORT_SYMBOL_GPL(PageHuge);
  */
 int PageHeadHuge(struct page *page_head)
 {
-	compound_page_dtor *dtor;
-
 	if (!PageHead(page_head))
 		return 0;
 
-	dtor = get_compound_page_dtor(page_head);
-
-	return dtor == free_huge_page;
+	return get_compound_page_dtor(page_head) == free_huge_page;
 }
 EXPORT_SYMBOL_GPL(PageHeadHuge);
 
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -82,19 +82,6 @@ static void __put_compound_page(struct page *page)
>  
>  static void put_compound_page(struct page *page)

This function has become quite crazy.  I sat down to refamiliarize but
immediately failed.

: static void put_compound_page(struct page *page)
: {
: 	if (unlikely(PageTail(page))) {
:	...
: 	} else if (put_page_testzero(page)) {
: 		if (PageHead(page))

How can a page be both PageTail() and PageHead()?

: 			__put_compound_page(page);
: 		else
: 			__put_single_page(page);
: 	}
: }
: 
: 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/