Date:   Wed, 20 Jul 2022 13:56:35 -0700
From:   Nadav Amit <nadav.amit@...il.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Peter Xu <peterx@...hat.com>, Linux MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andrew Cooper <andrew.cooper3@...rix.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will@...nel.org>, Yu Zhao <yuzhao@...gle.com>,
        Nick Piggin <npiggin@...il.com>
Subject: Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on
 writeprotect

On Jul 20, 2022, at 1:38 PM, David Hildenbrand <david@...hat.com> wrote:

> On 20.07.22 22:22, Nadav Amit wrote:
>> On Jul 20, 2022, at 12:55 PM, David Hildenbrand <david@...hat.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

Do you need both conditions to be true? I thought (a) is sufficient (if
the soft-dirty and related checks succeed).

> 
> 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.

That’s all I wanted to do there.

> 
>> 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 ... :)

I don’t know which arch you have in your mind. But the moment a PTE is
writable, then marking it logically/architecturally as dirty should be
fine.

But… if the Exclusive check is not good enough for private+anon without
the “logical” dirty bit, then there would be a problem. 

Powered by blists - more mailing lists