Message-ID: <c7fc5bc3-7f86-c699-2968-a861ab44989a@redhat.com>
Date:   Thu, 2 Mar 2023 15:12:55 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Axel Rasmussen <axelrasmussen@google.com>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Muhammad Usama Anjum <usama.anjum@collabora.com>
References: <Y+5uIS5E9sTLi41T@x1n>
 <456f8e2e-9554-73a3-4fdb-be21f9cc54b6@redhat.com> <Y+6NKPuty9V3nycI@x1n>
 <4dbc9913-3483-d22d-bbd2-e4f510fff56d@redhat.com> <Y/AIEvHJoMUnh4b6@x1n>
 <c463c421-2f33-9ae3-7d41-b394d1737d42@redhat.com> <Y/VQMyssclyIGa5i@x1n>
 <91d7c512-ee57-7d71-34b7-90e45f5c109b@redhat.com> <Y/Z9ACW8l2E3kOVk@x1n>
 <4b3c2f37-3b84-3147-7513-4293e5408fdd@redhat.com> <Y/5ZNTESKfntBSF3@x1n>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH] mm/uffd: UFFD_FEATURE_WP_ZEROPAGE
In-Reply-To: <Y/5ZNTESKfntBSF3@x1n>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

>>
>> uffd-wp protecting a range:
>> * !pte_none() -> set uffd-wp bit and wrprotect
>> * pte_none() -> nothing to do
>> * PTE_UFFD_WP -> nothing to do
>> * PTE_UFFD_NO_WP -> set PTE_UFFD_WP
>>
>> unmapping a page (old way: !pte_none() -> pte_none()):
>> * uffd-wp bit set: set PTE_UFFD_WP
>> * uffd-wp bit not set: set PTE_UFFD_NO_WP
>>
>> (re)mapping a page (old: pte_none() -> !pte_none()):
>> * PTE_UFFD_WP -> set pte bit for new PTE
>> * PTE_UFFD_NO_WP -> don't set pte bit for new PTE
>> * pte_none() -> set pte bit for new PTE
>>
>> Zapping an anon page using MADV_DONTNEED is a bit confusing. It's actually
>> similar to a memory write (-> write zeroes), but we don't notify uffd-wp for
>> that (I think that's something you comment on below). Theoretically, we'd
>> want to set PTE_UFFD_NO_WP ("dirty") in the async mode. But that might need
>> more thought of what the expected semantics actually are.
>>
>> When we walk over the page tables we would get the following information
>> after protecting the range:
>>
>> * PTE_UFFD_WP -> clean, not modified since last protection round
>> * PTE_UFFD_NO_WP -> dirty, modified since last protection round
>> * pte_none() -> not mapped and therefore not modified since beginning of
>>                  protection.
>> * !pte_none() -> uffd-wp bit decides
> 
> I can't say I thought a lot but I feel like it may work. I'd probably avoid
> calling it PTE_UFFD_NO_WP or it'll be confusing.. maybe WP_WRITTEN or
> WP_RESOLVED instead.

I haven't thought about this further, but I maybe we might be able to 
just use a single PTE marker , because pte_none() would translate to 
PTE_UFFD_WP in such VMAs. So we could make the meaning of the 
PTE_UFFD_WP marker simply depend on the type of VMA.

If I am not wrong, we could stop setting PTE_UFFD_WP completely then, 
for any memory type (shmem/anon/hugetlb).

> 
> But that interface looks weird in that the protection happens right after
> VM_UFFD_WP applied to VMA and that keeps true until unregister.  One needs
> to reprotect using ioctl(UFFDIO_WRITEPROTECT) OTOH after the 1st round of
> tracking.  It just looks a little bit over-complicated, not to mention we
> will need two markers only for userfault-wp.  I had a feeling this
> complexity can cause us some trouble elsewhere.

When to apply this logic is indeed the interesting part. Doing it right 
from the beginning when the feature is enabled sounds like the simplest 
approach to me.  For background snapshots and dirty tracking that might 
just be good enough.


> 
> IIUC this can be something done on top even if it'll work (I think the

I think the API would have to change eventually, to enable it via a new 
feature ("unpopulated implies uffd-wp is active").

> userspace API doesn't need to change at all), so I'd suggest giving it some
> more thoughts and we start with simple and working.

Yes.

-- 
Thanks,

David / dhildenb