Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect

[David Hildenbrand <david@redhat.com>]

On 20.07.22 22:22, Nadav Amit wrote:
> On Jul 20, 2022, at 12:55 PM, David Hildenbrand <david@redhat.com> wrote:
> 
>> On 20.07.22 21:48, Peter Xu wrote:
>>> On Wed, Jul 20, 2022 at 09:33:35PM +0200, David Hildenbrand wrote:
>>>> On 20.07.22 21:15, Peter Xu wrote:
>>>>> On Wed, Jul 20, 2022 at 05:10:37PM +0200, David Hildenbrand wrote:
>>>>>> For pagecache pages it may as well be *plain wrong* to bypass the write
>>>>>> fault handler and simply mark pages dirty+map them writable.
>>>>>
>>>>> Could you elaborate?
>>>>
>>>> Write-fault handling for some filesystems (that even require this
>>>> "slow path") is a bit special.
>>>>
>>>> For example, do_shared_fault() might have to call page_mkwrite().
>>>>
>>>> AFAIK file systems use that for lazy allocation of disk blocks.
>>>> If you simply go ahead and map a !dirty pagecache page writable
>>>> and mark it dirty, it will not trigger page_mkwrite() and you might
>>>> end up corrupting data.
>>>>
>>>> That's why we the old change_pte_range() code never touched
>>>> anything if the pte wasn't already dirty.
>>>
>>> I don't think that pte_dirty() check was for the pagecache code. For any fs
>>> that has page_mkwrite() defined, it'll already have vma_wants_writenotify()
>>> return 1, so we'll never try to add write bit, hence we'll never even try
>>> to check pte_dirty().
>>
>> I might be too tired, but the whole reason we had this magic before my
>> commit in place was only for the pagecache.
>>
>> With vma_wants_writenotify()=0 you can directly map the pages writable
>> and don't have to do these advanced checks here. In a writable
>> MAP_SHARED VMA you'll already have pte_write().
>>
>> We only get !pte_write() in case we have vma_wants_writenotify()=1 ...
>>
>>  try_change_writable = vma_wants_writenotify(vma, vma->vm_page_prot);
>>
>> and that's the code that checked the dirty bit after all to decide --
>> amongst other things -- if we can simply map it writable without going
>> via the write fault handler and triggering do_shared_fault() .
>>
>> See crazy/ugly FOLL_FORCE code in GUP that similarly checks the dirty bit.
> 
> I thought you want to get rid of it at least for anonymous pages. No?

Yes. Especially for any MAP_PRIVATE mappings.

If you want to write to something that's not mapped writable in a
MAP_PRIVATE mapping it
a) Has to be an exclusive anonymous page
b) The pte has to be dirty

In any other case, you clearly missed to COW or the modifications might
get lost if the PTE is not dirty.

MAP_SHARED is a bit more involved. But whether the pte is dirty might be
good enough ... but this needs a lot more care.

> 
>>
>> But yeah, it's all confusing so I might just be wrong regarding
>> pagecache pages.
> 
> Just to note: I am not very courageous and I did not intend to change
> condition for when non-anonymous pages are set as writable. That’s the
> reason I did not change the dirty for non-writable non-anonymous entries (as
> Peter said). And that’s the reason that setting the dirty bit (at least as I
> should have done it) is only performed after we made the decision on the
> write-bit.

Good. As long as we stick to anonymous pages I roughly know what we we
can and cannot do at this point :)


The problem I see is that detection whether we can write requires the
dirty bit ... and whether to set the dirty bit requires the information
whether we can write.

Again, for anonymous pages we should be able to relax the "dirty"
requirement when detecting whether we can write.

> 
> IOW, after you made your decision about the write-bit, then and only then
> you may be able to set the dirty bit for writable entries. Since the entry
> is already writeable (i.e., can be written without a fault later directly
> from userspace), there should be no concern of correctness when you set it.

That makes sense to me. What keeps confusing me are architectures with
and without a hw-managed dirty bit ... :)

-- 
Thanks,

David / dhildenb