linux-kernel - Re: [PATCH RFC] mm/userfaultfd: enable writenotify while userfaultfd-wp is enabled for a VMA

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, 6 Dec 2022 16:27:31 -0500
From:   Peter Xu <peterx@...hat.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Ives van Hoorne <ives@...esandbox.io>,
        stable@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hugh@...itas.com>,
        Alistair Popple <apopple@...dia.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH RFC] mm/userfaultfd: enable writenotify while
 userfaultfd-wp is enabled for a VMA

On Tue, Dec 06, 2022 at 05:28:07PM +0100, David Hildenbrand wrote:
> > If no one is using mprotect() with uffd-wp like that, then the reproducer
> > may not be valid - the reproducer is defining how it should work, but does
> > that really stand?  That's why I said it's ambiguous, because the
> > definition in this case is unclear.
> 
> There are interesting variations like:
> 
> mmap(PROT_READ, MAP_POPULATE|MAP_SHARED)
> uffd_wp()
> mprotect(PROT_READ|PROT_WRITE)
> 
> Where we start out with all-write permissions before we enable selective
> write permissions.

Could you elaborate what's the difference of above comparing to:

mmap(PROT_READ|PROT_WRITE, MAP_POPULATE|MAP_SHARED)
uffd_wp()

?

[...]

> Yes, you are correct. I added that to the patch description:
> 
> "
> Note that we don't optimize for the actual migration case:
>     (1) When migration succeeds the new PTE will not be writable because
>         the source PTE was not writable (protnone); in the future we
>         might just optimize that case similarly by reusing
>         can_change_pte_writable()/can_change_pmd_writable() when
>         removing migration PTEs.
>     (2) When migration fails, we'd have to recalculate the "writable"
>         flag because we temporarily dropped the PT lock; for now keep it
>         simple and set "writable=false".
> "
> 
> Case (1) would, with your current patch, always lose the write bit during
> migration, even if vma->vm_page_prot included it. We most might want to
> optimize that in the future.
> 
> Case (2) is rather a corner case, and unless people complain about it being
> a real performance issue, it felt cleaner (less code) to not optimize for
> that now.

As I didn't have a closer look on the savedwrite removal patchset so I may
not speak anything sensible here..  What I hope is that we don't lose write
bits easily, after all we tried to even safe the dirty and young bits to
avoid the machine cycles in the MMUs.

> 
> Again Peter, I am not against you, not at all. Sorry if I gave you the
> impression. I highly appreciate your work and this discussion.

No worry on that part.  You're doing great in this email explaining things
and write things up, especially I'm happy Hugh confirmed it so it's good to
have those.  Let's start with something like this when you NAK something
next time. :)

Thanks,

-- 
Peter Xu