linux-kernel - Re: [PATCH 2/2] mm/khugepaged: skip shmem with userfaultfd

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: <CAHbLzkokih0j=niLuWJLBDfTS8biiDTjC6gudw+qcDCBpMneAA@mail.gmail.com>
Date:   Thu, 16 Feb 2023 15:52:37 -0800
From:   Yang Shi <shy828301@...il.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     David Stevens <stevensd@...omium.org>, linux-mm@...ck.org,
        Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        David Hildenbrand <david@...hat.com>,
        Hugh Dickins <hughd@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mm/khugepaged: skip shmem with userfaultfd

On Thu, Feb 16, 2023 at 3:07 PM Peter Xu <peterx@...hat.com> wrote:
>
> Hi, Yang,
>
> On Thu, Feb 16, 2023 at 01:58:55PM -0800, Yang Shi wrote:
> > > IIUC we released it before copying the pages:
> >
> > The huge page is locked until the copy is done. It should be fine
> > unless the users inspect the page content without acquiring page lock.
>
> The current patch from David has replaced "insert hpage into holes" with
> "insert RETRY entries into holes", so IMHO the hpage is not visible at all
> when releasing page cache lock here.

IIRC his patch (just this patch, don't include patch #1) conceptually does:

acquire xa lock

fill the holes with retry entry
if (nr_none == nr_none_check && uffd missing pass) /* no hole is
filled since holding xa_lock and no uffd missing */
    install huge page in page cache <-- huge page is visible here
else {
    set error code
    replace retry entry back to NULL
}

release xa_lock

if (succeed) {
    copy content to huge page
    unlock huge page
} else
    restore the small pages


Am I missing something?

>
> All the accessors (including RCU protected ones to access page cache; those
> may not need to take the page lock) should be spinning on the RETRY entry,
> which it seems fine to me.  But my question was whether it's legal to keep
> them spinning even after releasing the page cache lock.

After releasing the page cache lock, they should see NULL entry or
huge page IIUC.

>
> Thanks,
>
> >
> > >
> > > xa_locked:
> > >         xas_unlock_irq(&xas);  <-------------------------------- here
> > > xa_unlocked:
> > >
> > >         /*
> > >          * If collapse is successful, flush must be done now before copying.
> > >          * If collapse is unsuccessful, does flush actually need to be done?
> > >          * Do it anyway, to clear the state.
> > >          */
> > >         try_to_unmap_flush();
> > >
> > > Before insertion of the multi-index:
> > >
> > >         /* Join all the small entries into a single multi-index entry. */
> > >         xas_set_order(&xas, start, HPAGE_PMD_ORDER);
> > >         xas_store(&xas, hpage);
> > >
> > > Thanks,
> > >
> > > --
> > > Peter Xu
> > >
> >
>
> --
> Peter Xu
>