linux-kernel - Re: [External] [PATCH 3/3] hugetlb: before freeing hugetlb page set dtor to appropriate value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Jul 2021 18:57:36 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Michal Hocko <mhocko@...e.com>,
        Oscar Salvador <osalvador@...e.de>,
        David Hildenbrand <david@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        Mina Almasry <almasrymina@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [External] [PATCH 3/3] hugetlb: before freeing hugetlb page set
 dtor to appropriate value

On Sat, Jul 10, 2021 at 8:25 AM Mike Kravetz <mike.kravetz@...cle.com> 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@...cle.com>
> ---
>  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
>