linux-kernel - Re: [PATCH 2/8] mm: khugepaged: remove redundant check for VM_NO

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: <CAHbLzkqNq_H6Cdwzd-3VazsQbEz=ZUFU60ec+Py8w3nhx7E5pw@mail.gmail.com>
Date:   Wed, 2 Mar 2022 10:43:31 -0800
From:   Yang Shi <shy828301@...il.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     Linux MM <linux-mm@...ck.org>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Song Liu <songliubraving@...com>,
        Rik van Riel <riel@...riel.com>,
        Matthew Wilcox <willy@...radead.org>, Zi Yan <ziy@...dia.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Theodore Ts'o" <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        darrick.wong@...cle.com
Subject: Re: [PATCH 2/8] mm: khugepaged: remove redundant check for VM_NO_KHUGEPAGED

On Tue, Mar 1, 2022 at 1:07 AM Miaohe Lin <linmiaohe@...wei.com> wrote:
>
> On 2022/3/1 7:57, Yang Shi wrote:
> > The hugepage_vma_check() called by khugepaged_enter_vma_merge() does
> > check VM_NO_KHUGEPAGED. Remove the check from caller and move the check
> > in hugepage_vma_check() up.
> >
> > More checks may be run for VM_NO_KHUGEPAGED vmas, but MADV_HUGEPAGE is
> > definitely not a hot path, so cleaner code does outweigh.
> >
> > Signed-off-by: Yang Shi <shy828301@...il.com>
> > ---
> >  mm/khugepaged.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 131492fd1148..82c71c6da9ce 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -366,8 +366,7 @@ int hugepage_madvise(struct vm_area_struct *vma,
> >                * register it here without waiting a page fault that
> >                * may not happen any time soon.
> >                */
> > -             if (!(*vm_flags & VM_NO_KHUGEPAGED) &&
> > -                             khugepaged_enter_vma_merge(vma, *vm_flags))
> > +             if (khugepaged_enter_vma_merge(vma, *vm_flags))
> >                       return -ENOMEM;
> >               break;
> >       case MADV_NOHUGEPAGE:
> > @@ -446,6 +445,9 @@ static bool hugepage_vma_check(struct vm_area_struct *vma,
> >       if (!transhuge_vma_enabled(vma, vm_flags))
> >               return false;
> >
> > +     if (vm_flags & VM_NO_KHUGEPAGED)
> > +             return false;
> > +
>
> This patch does improve the readability. But I have a question.
> It seems VM_NO_KHUGEPAGED is not checked in the below if-condition:
>
>         /* Only regular file is valid */
>         if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && vma->vm_file &&
>             (vm_flags & VM_EXEC)) {
>                 struct inode *inode = vma->vm_file->f_inode;
>
>                 return !inode_is_open_for_write(inode) &&
>                         S_ISREG(inode->i_mode);
>         }
>
> If we return false due to VM_NO_KHUGEPAGED here, it seems it will affect the
> return value of this CONFIG_READ_ONLY_THP_FOR_FS condition check.
> Or am I miss something?

Yes, it will return false instead of true if that file THP check is
true, but wasn't that old behavior actually problematic? Khugepaged
definitely can't collapse VM_NO_KHUGEPAGED vmas even though it
satisfies all the readonly file THP checks. With the old behavior
khugepaged may scan an exec file hugetlb vma IIUC although it will
fail later due to other page sanity checks, i.e. page compound check.

>
> Thanks.
>
> >       if (vma->vm_file && !IS_ALIGNED((vma->vm_start >> PAGE_SHIFT) -
> >                               vma->vm_pgoff, HPAGE_PMD_NR))
> >               return false;
> > @@ -471,7 +473,8 @@ static bool hugepage_vma_check(struct vm_area_struct *vma,
> >               return false;
> >       if (vma_is_temporary_stack(vma))
> >               return false;
> > -     return !(vm_flags & VM_NO_KHUGEPAGED);
> > +
> > +     return true;
> >  }
> >
> >  int __khugepaged_enter(struct mm_struct *mm)
> >
>
>