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]

Message-ID: <22d8e8ac-d75-a66-2650-b4d59f89855e@google.com>
Date:   Tue, 6 Dec 2022 11:09:40 -0800 (PST)
From:   Hugh Dickins <hughd@...gle.com>
To:     Peter Xu <peterx@...hat.com>
cc:     David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Ives van Hoorne <ives@...esandbox.io>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...le.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, 6 Dec 2022, David Hildenbrand wrote:
...
> 
> We never had to remove write permissions so far from the vma->vm_page_prot
> default. We always only added permissions.
> 
> 
> Now, uffd-wp on shmem as of now violates these semantics. vma->vm_page_prot
> cannot always be used as a safe default, because we might have to wrprotect
> individual PTEs. Note that for uffd-wp on anonymous memory this was not an
> issue, because we default to !write in vma->vm_page_prot.
> 
> 
> The two possible ways to approach this for uffd-wp on shmem are:
> 
> (1) Obey existing vma->vm_page_prot semantics. Default to !write and
>     optimize the relevant cases to *add* the write bit. This is
>     essentially what this patch does, minus
>     can_change_pte_writable() optimizations on relevant code paths where
>     we'd want to maintain the write bit. For example, when removing
>     uffd-wp protection we might want to restore the write bit directly.
> 
> (2) Default to write permissions and check each and every code location
>     where we remap for uffd-wp ptes, to manuall wrprotect -- *remove*
>     the write bit. Alternatively, as you said, always wrprotect when
>     setting the PTE bit, which might work as well.
> 
> 
> My claim is that (1) is less error prone, because in the worst case we forget
> to optimize one code location -- instead to resulting in a BUG if we forget to
> wrprotect (what we have now). But I am not going to fight for it, because I
> can see that (2) can be made to work as well, as you outline in your patch.
> 
> You seem to have decided on (2). Fair enough, you know uffd-wp best. We just
> have to fix it properly and make the logic consistent whenever we remap a
> page.
> 
...
> 
> But I'm not going to argue about whats valid and whats not as long as it's
> documented ;). I primarily wanted to showcase that the same logic based on
> vma->vm_page_prot is used elsewhere, and that migration PTE restoration is not
> particularly special.

I have not been following the uffd-wp work, but I believe that David's
painstaking and excellent account of vm_page_prot is correct.  Peter,
please I beg you to follow his advice and go for (1) for uffd-wp.

I do not share David's faith in "documented": documented or not,
depart from safe convention and you will be adding (at least the
opportunity for) serious bugs.

Hugh