Re: [RFC][PATCH v2 1/5] mm: Avoid unmapping pinned pages

From: David Hildenbrand <david@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: mingo@redhat.com, tglx@linutronix.de, juri.lelli@redhat.com,
	vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
	rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
	bristot@redhat.com, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-api@vger.kernel.org, x86@kernel.org,
	pjt@google.com, posk@google.com, avagin@google.com,
	jannh@google.com, tdelisle@uwaterloo.ca, mark.rutland@arm.com,
	posk@posk.io
Subject: Re: [RFC][PATCH v2 1/5] mm: Avoid unmapping pinned pages
Date: Fri, 21 Jan 2022 13:04:09 +0100	[thread overview]
Message-ID: <7744b904-4803-1c8e-3a1c-eebd30f2da91@redhat.com> (raw)
In-Reply-To: <20220121114058.GE20638@worktop.programming.kicks-ass.net>

On 21.01.22 12:40, Peter Zijlstra wrote:
> On Fri, Jan 21, 2022 at 10:04:45AM +0100, David Hildenbrand wrote:
>> On 21.01.22 09:59, Peter Zijlstra wrote:
> 
>>> However, I'm not quite sure what fork() does with pages that have a pin.
>>
>> We COW the anon pages always, and we protect against concurrent GUP
>> using the
>> * mmap_lock in exclusive mode for ordinary GUP
>> * mm->write_protect_seq for GUP-fast
> 
> Right, but neither the mmap_sem nor the write_protect_seq help anything
> at all vs already extant page pins.
> 
> But I just found copy_present_page()'s page_needs_cow_for_dma(), which I
> think deals with exactly that case, it avoids doing CoW on pinned pages
> and instead feeds the child a full copy while keeping the pinned page in
> the original process.

Yes, page_needs_cow_for_dma() is the magic bit. The locking I explained
keep its output "reliable".

> 
>>> Naively, a page that has async DMA activity should not be CoW'ed, or if
>>> it is, care must be taken to ensure the original pages stays in the
>>> original process, but I realize that's somewhat hard.
>>
>> That's precisely what I'm working on fixing ... and yes, it's hard.
>>
>> Let me know if you need any other information, I've spent way too much
>> time on this than I ever panned.
> 
> So let me try and get this right:
> 
>  - GUP post-fork breaks CoW for FOLL_WRITE/FOLL_PIN, without either
>    there's a problem where one task might observe changes by another.
> 
>  - GUP pre-fork prevents CoW and does a full copy.

Yes, pretty much.

> 
> And that all mostly works, except for a fair amount of 'fun' cases?

I'd say some obviously broken cases, some racy cases, some fun cases :)

We have three main cases. And usually, trying to tackle one triggers
another.

(1) Missed CoW

If the child R/O pins and unmaps the page, the parent might miss to CoW
and reuse the page. Security issue. Once CVE in that area is currently
still applicable for THP (well, and hugetlb).

(2) Unnecessary CoW

We CoW instead of reusing the page, but there are no relevant pins, so
it's unnecessary.

(3) Wrong CoW

We CoW a page that has relevant pins, losing synchronicity between GUP
and the page tables.

The "criticality" is (1), (3), (2).

(3) can currently get triggered by anything that can map a pinned page
R/O. The racy case is what I described about the swapcache. Other broken
cases are mprotect() and friends (we cannot differentiate between R/O
and R/W pins ...).

-- 
Thanks,

David / dhildenb