On Thursday, 22 July 2021 7:35:32 AM AEST Peter Xu wrote:

On Wed, Jul 21, 2021 at 09:28:49PM +1000, Alistair Popple wrote:

quoted

On Saturday, 17 July 2021 5:11:33 AM AEST Peter Xu wrote:

quoted

On Fri, Jul 16, 2021 at 03:50:52PM +1000, Alistair Popple wrote:

quoted

Hi Peter,

[...]

quoted

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ae1f5d0cb581..4b46c099ad94 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c

@@ -5738,7 +5738,7 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma,
 
 	if (pte_present(ptent))
 		page = mc_handle_present_pte(vma, addr, ptent);
-	else if (is_swap_pte(ptent))
+	else if (pte_has_swap_entry(ptent))
 		page = mc_handle_swap_pte(vma, ptent, &ent);
 	else if (pte_none(ptent))
 		page = mc_handle_file_pte(vma, addr, ptent, &ent);

As I understand things pte_none() == False for a special swap pte, but
shouldn't this be treated as pte_none() here? Ie. does this need to be
pte_none(ptent) || is_swap_special_pte() here?

Looks correct; here the page/swap cache could hide behind the special pte just
like a none pte.  Will fix it.  Thanks!

quoted

quoted

diff --git a/mm/memory.c b/mm/memory.c
index 0e0de08a2cd5..998a4f9a3744 100644
--- a/mm/memory.c
+++ b/mm/memory.c

@@ -3491,6 +3491,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	if (!pte_unmap_same(vmf))
 		goto out;
 
+	/*
+	 * We should never call do_swap_page upon a swap special pte; just be
+	 * safe to bail out if it happens.
+	 */
+	if (WARN_ON_ONCE(is_swap_special_pte(vmf->orig_pte)))
+		goto out;
+
 	entry = pte_to_swp_entry(vmf->orig_pte);
 	if (unlikely(non_swap_entry(entry))) {
 		if (is_migration_entry(entry)) {

Are there other changes required here? Because we can end up with stale special
pte's and a special pte is !pte_none don't we need to fix some of the !pte_none
checks in these functions:

insert_pfn() -> checks for !pte_none
remap_pte_range() -> BUG_ON(!pte_none)
apply_to_pte_range() -> didn't check further but it tests for !pte_none

In general it feels like I might be missing something here though. There are
plenty of checks in the kernel for pte_none() which haven't been updated. Is
there some rule that says none of those paths can see a special pte?

My rule on doing this was to only care about vma that can be backed by RAM,
majorly shmem/hugetlb, so the special pte can only exist there within those
vmas.  I believe in most pte_none() users this special pte won't exist.

So if it's not related to RAM backed memory at all, maybe it's fine to keep the
pte_none() usage like before.

Take the example of insert_pfn() referenced first - I think it can be used to
map some MMIO regions, but I don't think we'll call that upon a RAM region
(either shmem or hugetlb), nor can it be uffd wr-protected.  So I'm not sure
adding special pte check there would be helpful.

apply_to_pte_range() seems to be a bit special - I think the pte_fn_t matters
more on whether the special pte will matter.  I had a quick look, it seems
still be used mostly by all kinds of driver code not mm core.  It's used in two
forms:

        apply_to_page_range
        apply_to_existing_page_range

The first one creates ptes only, so it ignores the pte_none() check so I skipped.

The second one has two call sites:

*** arch/powerpc/mm/pageattr.c:
change_memory_attr[99]         return apply_to_existing_page_range(&init_mm, start, size,
set_memory_attr[132]           return apply_to_existing_page_range(&init_mm, start, sz, set_page_attr,

*** mm/kasan/shadow.c:
kasan_release_vmalloc[485]     apply_to_existing_page_range(&init_mm,

I'll leave the ppc callers for now as uffd-wp is not even supported there.  The
kasan_release_vmalloc() should be for kernel allocated memories only, so should
not be a target for special pte either.

So indeed it's hard to 100% cover all pte_none() users to make sure things are
used right.  As stated above I still believe most callers don't need that, but
the worst case is if someone triggered uffd-wp issues with a specific feature,
we can look into it.  I am not sure whether it's good we add this for all the
pte_none() users, because mostly they'll be useless checks, imho.

I wonder then - should we make pte_none() return true for these special pte's
as well? It seems if we do miss any callers it could result in some fairly hard
to find bugs if the code follows a different path due to the presence of an
unexpected special pte changing the result of pte_none().

I thought about something similar before, but I didn't dare to change
pte_none() as it's been there for ages and I'm afraid people will get confused
when it's meaning changed.  So even if we want to have some helper identifying
"either none pte or the swap special pte" it should use a different name.

Modifying the meaning of pte_none() could also have other risks that when we
really want an empty pte to be doing something else now.  It turns out there's
no easy way to not identify the case one by one, at least to me.  I'm always
open to good suggestions.

I'm not convinced it's changing the behaviour of pte_none() though and my
concern is that introducing special swap ptes does change it. Prior to this
clearing a pte would result in pte_none()==True. After this series clearing a
pte can some sometimes result in pte_none()==False because it doesn't really
get cleared.

Now as you say it's hard to cover 100% of pte_none() uses, so it's possible we
have missed cases that may now encounter a special pte and take a different
path (get_mctgt_type() is one example, I stopped looking for other possible
ones after mm/memory.c).

So perhaps if we want to keep pte_none() to check for really clear pte's then
what is required is converting all callers to a new helper
(pte_none_not_special()?) that treats special swap ptes as pte_none() and warns
if a special pte is encountered?

Btw, as you mentioned before, we can use a new number out of MAX_SWAPFILES,
that'll make all these easier a bit here, then we don't need to worry on
pte_none() issues too.  Two days ago Hugh has raised some similar concern on
whether it's good to implement this uffd-wp special pte like this.  I think we
can discuss this separately.

Yes, I saw that and personally I still prefer that approach.

quoted

quoted

So far what I planned to do is to cover most things we know that may be
affected like this patch so the change may bring a difference, hopefully we
won't miss any important spots.

quoted

quoted

diff --git a/mm/migrate.c b/mm/migrate.c
index 23cbd9de030b..b477d0d5f911 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c

@@ -294,7 +294,7 @@ void __migration_entry_wait(struct mm_struct *mm, pte_t *ptep,
 
 	spin_lock(ptl);
 	pte = *ptep;
-	if (!is_swap_pte(pte))
+	if (!pte_has_swap_entry(pte))
 		goto out;
 
 	entry = pte_to_swp_entry(pte);

@@ -2276,7 +2276,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 
 		pte = *ptep;
 
-		if (pte_none(pte)) {
+		if (pte_none(pte) || is_swap_special_pte(pte)) {

I was wondering if we can loose the special pte information here? However I see
that in migrate_vma_insert_page() we check again and fail the migration if
!pte_none() so I think this is ok.

I think it would be better if this check was moved below so the migration fails
early. Ie:

		if (pte_none(pte)) {
 			if (vma_is_anonymous(vma) && !is_swap_special_pte(pte)) {

Hmm.. but shouldn't vma_is_anonymous()==true already means it must not be a
swap special pte?  Because swap special pte only exists when !vma_is_anonymous().

Oh ok that makes sense. With the code written that way it is easy to forget
that though so maybe a comment would help?

I've put most words in comment of is_swap_special_pte().  Do you perhaps have a
suggestion on the comment here?

Perhaps something like "swap special ptes only exist for !vma_is_anonymous(vma)"?

And I now see my original code suggestion was wrong anyway :)

quoted

quoted

quoted

Also how does this work for page migration in general? I can see in
page_vma_mapped_walk() that we skip special pte's, but doesn't this mean we
loose the special pte in that instance? Or is that ok for some reason?

Do you mean try_to_migrate_one()? Does it need to be aware of that?  Per my
understanding that's only for anonymous private memory, while in that world
there should have no swap special pte (page_lock_anon_vma_read will return NULL
early for !vma_is_anonymous).

As far as I know try_to_migrate_one() gets called for both anonymous pages and
file-backed pages. page_lock_anon_vma_read() is only called in the case of an
anonymous vma. See the implementation of rmap_walk() - it will call either
rmap_walk_anon() or rmap_walk_file() depending on the result of PageAnon().

I may have replied too soon there. :)  I think you're right.

So I think how it should work with page migration is: we skip that pte just
like what you said (check_pte returns false), then the per-pte info will be
kept there, irrelevant of what's the backing page is.  When it faults, it'll
bring up with either the old/new page depending on migration finished or not.
Does that sound working to you?

Yes actually I think this is ok. check_pte returns false for special pte's so
the existing special pte will be left in place to be dealt with as normal.

 - Alistair

Thanks,