linux-kernel - Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA balancing shared exec THP case

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:   Tue, 23 Mar 2021 18:16:10 -0700
From:   Yang Shi <shy828301@...il.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Jerome Glisse <jglisse@...hat.com>,
        Rafael Aquini <aquini@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        Alistair Popple <apopple@...dia.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>
Subject: Re: [PATCH v2 5/5] mm/migrate.c: fix potential deadlock in NUMA
 balancing shared exec THP case

On Tue, Mar 23, 2021 at 10:17 AM Yang Shi <shy828301@...il.com> wrote:
>
> On Tue, Mar 23, 2021 at 6:55 AM Miaohe Lin <linmiaohe@...wei.com> wrote:
> >
> > Since commit c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA
> > balancing"), the NUMA balancing would skip shared exec transhuge page.
> > But this enhancement is not suitable for transhuge page. Because it's
> > required that page_mapcount() must be 1 due to no migration pte dance
> > is done here. On the other hand, the shared exec transhuge page will
> > leave the migrate_misplaced_page() with pte entry untouched and page
> > locked. Thus pagefault for NUMA will be triggered again and deadlock
> > occurs when we start waiting for the page lock held by ourselves.
>
> Thanks for catching this. By relooking the code I think the other
> important reason for removing this is
> migrate_misplaced_transhuge_page() actually can't see shared exec file
> THP at all since page_lock_anon_vma_read() is called before and if
> page is not anonymous page it will just restore the PMD without
> migrating anything.
>
> The pages for private mapped file vma may be anonymous pages due to
> COW but they can't be THP so it won't trigger THP numa fault at all. I
> think this is why no bug was reported. I overlooked this in the first
> place.
>
> Your fix is correct, and please add the above justification to your commit log.

BTW, I think you can just undo or revert commit c77c5cbafe54 ("mm:
migrate: skip shared exec THP for NUMA balancing").

Thanks,
Yang

>
> Reviewed-by: Yang Shi <shy828301@...il.com>
>
> >
> > Fixes: c77c5cbafe54 ("mm: migrate: skip shared exec THP for NUMA balancing")
> > Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
> > ---
> >  mm/migrate.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 5357a8527ca2..68bfa1625898 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -2192,9 +2192,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
> >         int page_lru = page_is_file_lru(page);
> >         unsigned long start = address & HPAGE_PMD_MASK;
> >
> > -       if (is_shared_exec_page(vma, page))
> > -               goto out;
> > -
> >         new_page = alloc_pages_node(node,
> >                 (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE),
> >                 HPAGE_PMD_ORDER);
> > @@ -2306,7 +2303,6 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm,
> >
> >  out_unlock:
> >         unlock_page(page);
> > -out:
> >         put_page(page);
> >         return 0;
> >  }
> > --
> > 2.19.1
> >