linux-kernel - Re: [External] Re: [PATCH 2/6] hugetlbfs: fix cannot migrate the fallocated HugeTLB page

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: <CAMZfGtXApP2k5AWK5ff5TWh+nkY1bHKbMimj4faFC8u6bUzMCQ@mail.gmail.com>
Date:   Tue, 5 Jan 2021 10:44:35 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        Andi Kleen <ak@...ux.intel.com>, mhocko@...e.cz,
        Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [External] Re: [PATCH 2/6] hugetlbfs: fix cannot migrate the
 fallocated HugeTLB page

On Tue, Jan 5, 2021 at 6:40 AM Mike Kravetz <mike.kravetz@...cle.com> wrote:
>
> On 1/3/21 10:58 PM, Muchun Song wrote:
> > Because we only can isolate a active page via isolate_huge_page()
> > and hugetlbfs_fallocate() forget to mark it as active, we cannot
> > isolate and migrate those pages.
> >
> > Fixes: 70c3547e36f5 (hugetlbfs: add hugetlbfs_fallocate())
> > Signed-off-by: Muchun Song <songmuchun@...edance.com>
> > ---
> >  fs/hugetlbfs/inode.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
>
> Good catch.  This is indeed an issue.
>
> >
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index b5c109703daa..2aceb085d202 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -737,10 +737,11 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
> >
> >               /*
> >                * unlock_page because locked by add_to_page_cache()
> > -              * page_put due to reference from alloc_huge_page()
> > +              * put_page() (which is in the putback_active_hugepage())
> > +              * due to reference from alloc_huge_page()
>
> Thanks for fixing the comment.
>
> >                */
> >               unlock_page(page);
> > -             put_page(page);
> > +             putback_active_hugepage(page);
>
> I'm curious why you used putback_active_hugepage() here instead of simply
> calling set_page_huge_active() before the put_page()?
>
> When the page was allocated, it was placed on the active list (alloc_huge_page).
> Therefore, the hugetlb_lock locking and list movement should not be necessary.

I agree with you. Because set_page_huge_active is not exported (static
function). Only exporting set_page_huge_active seems strange (leaving
clear_page_huge_active not export). This is just my opinion. What's
yours, Mike?

Thanks.

>
> --
> Mike Kravetz
>
> >       }
> >
> >       if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + len > inode->i_size)
> >