linux-kernel - Re: [External] Re: [PATCH] mm: hugetlb: fix a race between memory-failure/soft_offline and gather_surplus

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: <CAMZfGtWh4tRiMrOTLvv5GHM1JUCt9b+UHf_DwLev32S=+iLW8g@mail.gmail.com>
Date:   Wed, 21 Apr 2021 16:15:00 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Oscar Salvador <osalvador@...e.de>,
        Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>
Subject: Re: [External] Re: [PATCH] mm: hugetlb: fix a race between
 memory-failure/soft_offline and gather_surplus_pages

On Wed, Apr 21, 2021 at 4:03 PM Michal Hocko <mhocko@...e.com> wrote:
>
> [Cc Naoya]
>
> On Wed 21-04-21 14:02:59, Muchun Song wrote:
> > The possible bad scenario:
> >
> > CPU0:                           CPU1:
> >
> >                                 gather_surplus_pages()
> >                                   page = alloc_surplus_huge_page()
> > memory_failure_hugetlb()
> >   get_hwpoison_page(page)
> >     __get_hwpoison_page(page)
> >       get_page_unless_zero(page)
> >                                   zero = put_page_testzero(page)
> >                                   VM_BUG_ON_PAGE(!zero, page)
> >                                   enqueue_huge_page(h, page)
> >   put_page(page)
> >
> > The refcount can possibly be increased by memory-failure or soft_offline
> > handlers, we can trigger VM_BUG_ON_PAGE and wrongly add the page to the
> > hugetlb pool list.
>
> The hwpoison side of this looks really suspicious to me. It shouldn't
> really touch the reference count of hugetlb pages without being very
> careful (and having hugetlb_lock held). What would happen if the
> reference count was increased after the page has been enqueed into the
> pool? This can just blow up later.

If the page has been enqueued into the pool, then the page can be
allocated to other users. The page reference count will be reset to
1 in the dequeue_huge_page_node_exact(). Then memory-failure
will free the page because of put_page(). This is wrong. Because
there is another user.

>
> > Signed-off-by: Muchun Song <songmuchun@...edance.com>
> > ---
> >  mm/hugetlb.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 3476aa06da70..6c96332db34b 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -2145,17 +2145,14 @@ static int gather_surplus_pages(struct hstate *h, long delta)
> >
> >       /* Free the needed pages to the hugetlb pool */
> >       list_for_each_entry_safe(page, tmp, &surplus_list, lru) {
> > -             int zeroed;
> > -
> >               if ((--needed) < 0)
> >                       break;
> >               /*
> > -              * This page is now managed by the hugetlb allocator and has
> > -              * no users -- drop the buddy allocator's reference.
> > +              * The refcount can possibly be increased by memory-failure or
> > +              * soft_offline handlers.
> >                */
> > -             zeroed = put_page_testzero(page);
> > -             VM_BUG_ON_PAGE(!zeroed, page);
> > -             enqueue_huge_page(h, page);
> > +             if (likely(put_page_testzero(page)))
> > +                     enqueue_huge_page(h, page);
> >       }
> >  free:
> >       spin_unlock_irq(&hugetlb_lock);
> > --
> > 2.11.0
> >
>
> --
> Michal Hocko
> SUSE Labs