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:   Wed, 20 Jul 2022 15:48:27 -0400
From:   Peter Xu <peterx@redhat.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     Nadav Amit <nadav.amit@gmail.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Rapoport <rppt@linux.ibm.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Nadav Amit <namit@vmware.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Will Deacon <will@kernel.org>, Yu Zhao <yuzhao@google.com>,
        Nick Piggin <npiggin@gmail.com>
Subject: Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on
 writeprotect
Message-ID: <YthcC78q1hdd7mNT@xz-m1.local>
References: <20220718120212.3180-1-namit@vmware.com>
 <20220718120212.3180-2-namit@vmware.com>
 <YtcYVMoSRVxRH70Z@xz-m1.local>
 <017facf0-7ef8-3faf-138d-3013a20b37db@redhat.com>
 <Ytf+zIxVPTVXTZdp@xz-m1.local>
 <2b4393ce-95c9-dd3e-8495-058a139e771e@redhat.com>
 <YthUYF3invrjlzUc@xz-m1.local>
 <69022bad-d6f1-d830-224d-eb8e5c90d5c7@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <69022bad-d6f1-d830-224d-eb8e5c90d5c7@redhat.com>
Precedence: bulk

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

> Because as long as it's not writable,
> the FS might have to be informed about the write-unprotect.
> 
> And we end up in the case here for VM_SHARED with vma_wants_writenotify().
> Where we, for example, check
> 
> /* The backer wishes to know when pages are first written to? *
> if (vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite))$
> 	return 1;
> 
> 
> Long story short, we should be really careful with write-fault handler bypasses,
> especially when deciding to set dirty bits. For pagecache pages, we have to be
> especially careful.

Since you mentioned page_mkwrite, IMHO it's really the write bit not dirty
bit that matters here (and IMHO that's why it's called page_mkwrite() not
page_mkdirty()).  Here Nadav's patch added pte_mkdirty() only if
pte_mkwrite() happens.  So I'm a bit confused on what's your worry, and
what you're against doing.

Say, even if with my original proposal to set dirty unconditionally, it'll
be still be after the pte_mkwrite().  I never see how that could affect
page_mkwrite not to mention it'll not even reach there.

> 
> For exclusive anon pages it's mostly ok, because wp_page_reuse()
> doesn't really contain that much magic. The only thing to consider for ordinary
> mprotect() is that there is -- IMHO -- absolutely no guarantee that someone will
> write to a specific write-unprotected page soon. For uffd-wp-unprotect it might be
> easier to guess, especially, if we un-protect only a single page.

Yeh, as mentioned I think that's a valid point - looks good to me to attach
the dirty bit only when with a hint.

Thanks,

-- 
Peter Xu