This was wrong from the very start. If we're not in GUP, we shouldn't call
GUP functions.
My understanding is !GET && !PIN is also called gup.. otherwise we don't
need GET and it can just be always implied.
That's not the point. The point is that _arbitrary_ code shouldn't call
into GUP internal helper functions, where they bypass, for example, any
sanity checks.
The other proof is try_grab_page() doesn't fail hard on !GET && !PIN. So I
don't know whether that's "wrong" to be used..
To me, that is arbitrary code using a GUP internal helper and,
therefore, wrong.
Back to the topic: I'd say either of the patches look good to solve the
problem. If p2pdma pages are mapped as PFNMAP/MIXEDMAP (?), I guess
vm_normal_page_pmd() proposed here will also work on it, so nothing I see
wrong on 2nd one yet.
It looks nicer indeed to not have FOLL_FORCE here, but it also makes me
just wonder whether we should document NUMA behavior for FOLL_* somewhere,
because we have an implication right now on !FOLL_FORCE over NUMA, which is
not obvious to me..
Yes, we probably should. For get_use_pages() and friends that behavior
was always like that and it makes sense: usually it represent
application behavior.
And to look more over that aspect, see follow_page(): previously we can
follow a page for protnone (as it never applies FOLL_NUMA) but now it won't
(it never applies FOLL_FORCE, either, so it seems "accidentally" implies
FOLL_NUMA now). Not sure whether it's intended, though..
That was certainly an oversight, thanks for spotting that. That patch
was not supposed to change semantics:
diff --git a/mm/gup.c b/mm/gup.c
index 76d222ccc3ff..ac926e19ff72 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -851,6 +851,13 @@ struct page *follow_page(struct vm_area_struct
*vma, unsigned long address,
if (WARN_ON_ONCE(foll_flags & FOLL_PIN))
return NULL;
+ /*
+ * In contrast to get_user_pages() and friends, we don't want to
+ * fail if the PTE is PROT_NONE: see gup_can_follow_protnone().
+ */
+ if (!(foll_flags & FOLL_WRITE))
+ foll_flags |= FOLL_FORCE;
+
page = follow_page_mask(vma, address, foll_flags, &ctx);
if (ctx.pgmap)
put_dev_pagemap(ctx.pgmap);
--
Cheers,
David / dhildenb
