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]

Message-ID: <CAMZfGtWjZAJfPdow2VgNL9T9jYdLLgekKnnQVtKyzaEwC4FfJQ@mail.gmail.com>
Date:   Thu, 15 Jul 2021 11:14:03 +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 Thu, Jul 15, 2021 at 1:39 AM Mike Kravetz <mike.kravetz@...cle.com> wrote:
>
> On 7/14/21 3:57 AM, Muchun Song wrote:
> > On Sat, Jul 10, 2021 at 8:25 AM Mike Kravetz <mike.kravetz@...cle.com> wrote:
> >> +       /*
> >> +        * 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);
> >
>
> I did hit that warning in my testing and thought about removing it.
> However, I decided to keep it because non-hugetlb code also makes use of
> alloc_contig_range/free_contig_range and it might be useful in those
> cases.
>
> My 'guess' is that the warning was added not because of temporary ref
> count increases but rather to point out any code that forgot to drop a
> reference.

Got it. At least this patch looks good to me. So

Reviewed-by: Muchun Song <songmuchun@...edance.com>

>
> BTW - It is not just the 'head' page which could trigger this warning, but
> any 'tail' page as well.  That is because we do not call free_contig_range
> with a compound page, but rather a group of pages all with ref count of
> at least one.

Right.

>
> I'm happy to remove the warning if people do not think it is generally
> useful.

For me, I suggest removing it. If someone has any ideas, please
let us know.

> --
> Mike Kravetz