Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Dec 2022 11:09:40 -0800 (PST)
From:   Hugh Dickins <hughd@google.com>
To:     Peter Xu <peterx@redhat.com>
cc:     David Hildenbrand <david@redhat.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, Ives van Hoorne <ives@codesandbox.io>,
        Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@goole.com>,
        Alistair Popple <apopple@nvidia.com>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Andrea Arcangeli <aarcange@redhat.com>
Subject: Re: [PATCH RFC] mm/userfaultfd: enable writenotify while userfaultfd-wp
 is enabled for a VMA
In-Reply-To: <92173bad-caa3-6b43-9d1e-9a471fdbc184@redhat.com>
Message-ID: <22d8e8ac-d75-a66-2650-b4d59f89855e@google.com>
References: <20221202122748.113774-1-david@redhat.com> <Y4oo6cN1a4Yz5prh@x1n> <690afe0f-c9a0-9631-b365-d11d98fdf56f@redhat.com> <19800718-9cb6-9355-da1c-c7961b01e922@redhat.com> <Y45duzmGGUT0+u8t@x1n> <92173bad-caa3-6b43-9d1e-9a471fdbc184@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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