On Thu, Jan 20, 2022 at 07:25:08PM +0100, David Hildenbrand wrote:
> On 20.01.22 16:55, Peter Zijlstra wrote:
> > Add a guarantee for Anon pages that pin_user_page*() ensures the
> > user-mapping of these pages stay preserved. In order to ensure this
> > all rmap users have been audited:
> >
> > vmscan: already fails eviction due to page_maybe_dma_pinned()
> >
> > migrate: migration will fail on pinned pages due to
> > expected_page_refs() not matching, however that is
> > *after* try_to_migrate() has already destroyed the
> > user mapping of these pages. Add an early exit for
> > this case.
> >
> > numa-balance: as per the above, pinned pages cannot be migrated,
> > however numa balancing scanning will happily PROT_NONE
> > them to get usage information on these pages. Avoid
> > this for pinned pages.
>
> page_maybe_dma_pinned() can race with GUP-fast without
> mm->write_protect_seq. This is a real problem for vmscan() with
> concurrent GUP-fast as it can result in R/O mappings of pinned pages and
> GUP will lose synchronicity to the page table on write faults due to
> wrong COW.
Urgh, so yeah, that might be a problem. Follow up code uses it like
this:
+/*
+ * Pinning a page inhibits rmap based unmap for Anon pages. Doing a load
+ * through the user mapping ensures the user mapping exists.
+ */
+#define umcg_pin_and_load(_self, _pagep, _member) \
+({ \
+ __label__ __out; \
+ int __ret = -EFAULT; \
+ \
+ if (pin_user_pages_fast((unsigned long)(_self), 1, 0, &(_pagep)) != 1) \
+ goto __out; \
+ \
+ if (!PageAnon(_pagep) || \
+ get_user(_member, &(_self)->_member)) { \
+ unpin_user_page(_pagep); \
+ goto __out; \
+ } \
+ __ret = 0; \
+__out: __ret; \
+})
And after that hard assumes (on the penalty of SIGKILL) that direct user
access works. Specifically it does RmW ops on it. So I suppose I'd
better upgrade that load to a RmW at the very least.
But is that sufficient? Let me go find that race you mention...
