On Thu, Jan 07, 2021 at 01:29:43PM -0800, Linus Torvalds wrote:

On Thu, Jan 7, 2021 at 12:59 PM Andrea Arcangeli [off-list ref] wrote:

quoted

The problem is it's not even possible to detect reliably if there's
really a long term GUP pin because of speculative pagecache lookups.

So none of the normal code _needs_ that any more these days, which is
what I think is so nice. Any pinning will do the COW, and then we have
the logic to make sure it stays writable, and that keeps everything
nicely coherent and is all fairly simple.

And yes, it does mean that if somebody then explicitly write-protects
a page, it may end up being COW'ed after all, but if you first pinned
it, and then started playing with the protections of that page, why
should you be surprised?

So to me, this sounds like a "don't do that then" situation.

Anybody who does page pinning and wants coherency should NOT TOUCH THE
MAPPING IT PINNED.

(And if you do touch it, it's your own fault, and you get to keep both
of the broken pieces)

Now, I do agree that from a QoI standpoint, it would be really lovely
if we actually enforced it. I'm not entirely sure we can, but maybe it
would be reasonable to use that

  mm->has_pinned && page_maybe_dma_pinned(page)

at least as the beginning of a heuristic.

In fact, I do think that "page_maybe_dma_pinned()" could possibly be
made stronger than it is. Because at *THAT* point, we might say "we
know a pinned page always must have a page_mapcount() of 1" - since as
part of pinning it and doing the GUP_PIN, we forced the COW, and then
subsequent fork() operations enforce it too.

So I do think that it might be possible to make that clear_refs code
notice "this page is pinned, I can't mark it WP without the pinning
coherency breaking".

It might not even be hard. But admittedly I'm somewhat handwaving
here, and I might not have thought of some situation.

I suppose the objective would be to detect when it's a transient pin
(as an O_DIRECT write) and fail clear_refs with an error for all other
cases of real long term pins that need to keep reading at full PCI
bandwidth, without extra GUP invocations after the wp_copy_page run.

Because of the speculative lookups are making the count unstable, it's
not even enough to use mmu notifier and never use FOLL_GET in GUP to
make it safe again (unlike what I mentioned in a previous email).

Random memory corruption will still silently materialize as result of
the speculative lookups in the above scenario.

My whole point here in starting this new thread to suggest page_count
in do_wp_page is an untenable solution is that such commit silently
broke every single long term PIN user that may be used in combination
of clear_refs since 2013.

Silent memory corruption undetected or a detectable error out of
clear_refs, are both different side effects the same technical ABI
break that rendered clear_refs fundamentally incompatible with
clear_refs, so detecting it or not still an ABI break that is.

I felt obliged to report there's something much deeper and
fundamentally incompatible between the page_count in do_wp_page any
wrprotection of exclusive memory in place, as if used in combination
with any RDMA for example. The TLB flushing and the
mmap_read/write_lock were just the tip of the iceberg and they're not
the main concern anymore.

In addition, the inefficiency caused by the fact the page_count effect
is multiplied by 512 times or 262144 while the mapcount remains 4k
granular, makes me think the page_count is unsuitable to be used there
and this specific cure with page_count in do_wp_page, looks worse than
the initial zygote disease.

Thanks,
Andrea