Re: [PATCH 0/2] page_count can't be used to decide when wp_page_copy

Andrea Arcangeli
2021-01-07T22:58:46Z

On Thu, Jan 07, 2021 at 02:17:50PM -0800, Linus Torvalds wrote:
> So I think we can agree that even that softdirty case we can just
> handle by "don't do that then".

Absolutely. The question is if somebody was happily running clear_refs
with a RDMA attached to the process, by the time they update and
reboot they'll find it the hard way with silent mm corruption
currently.

So I was obliged to report this issue and the fact there was very
strong reason why page_count was not used there and it's even
documented explicitly in the source:

* [..] however we only use
* page_trans_huge_mapcount() in the copy-on-write faults where we
* need full accuracy to avoid breaking page pinning, [..]

I didn't entirely forget the comment when I reiterated it in fact also
in Message-ID: <20200527212005.GC31990@redhat.com> on May 27 2020
since I recalled there was a very explicit reason why we had to use
page_mapcount in do_wp_page and deliver full accuracy.

Now I cannot proof there's any such user that will break, but we'll
find those with a 1 year or more of delay.

Even the tlb flushing deferral that caused clear_refs_write to also
corrupt userland memory and literally lose userland writes even
without any special secondary MMU hardware being attached to the
memory, took 6 months to find.

> if a page is pinned, the dirty bit of it makes no sense, because it
> might be dirtied complately asynchronously by the pinner.
>
> So I think none of the softdirty stuff should touch pinned pages. I
> think it falls solidly under that "don't do it then".
>
> Just skipping over them in clear_soft_dirty[_pmd]() doesn't look that
> hard, does it?

1) How do you know again if it's not speculative lookup or an O_DIRECT
that made them look pinned?

2) it's a hugepage 1, a 4k pin will make soft dirty then skip 511
dirty bits by mistake including those pages that weren't pinned and
that userland is still writing through the transhuge pmd.

Until v4.x we had a page pin counter for each subpage so the above
wouldn't have happened, but not anymore since it was simplified and
improved but now the page_count is pretty inefficient there, even
if it'd be possible to make safe.

3) the GUP(write=0) may be just reading from RAM and sending to the
other end so clear_refs may have currently very much tracked CPU
writes fine, with no interference whatsoever from the secondary MMU
working in readonly on the memory.

Thanks,
Andrea