linux-kernel - Race condition observed between page migration and page fault handling on arm64 machines

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240801081657.1386743-1-dev.jain@arm.com>
Date: Thu,  1 Aug 2024 13:46:57 +0530
From: Dev Jain <dev.jain@....com>
To: akpm@...ux-foundation.org,
	david@...hat.com,
	willy@...radead.org
Cc: ryan.roberts@....com,
	anshuman.khandual@....com,
	catalin.marinas@....com,
	cl@...two.org,
	vbabka@...e.cz,
	mhocko@...e.com,
	apopple@...dia.com,
	osalvador@...e.de,
	baolin.wang@...ux.alibaba.com,
	dave.hansen@...ux.intel.com,
	will@...nel.org,
	baohua@...nel.org,
	ioworker0@...il.com,
	gshan@...hat.com,
	mark.rutland@....com,
	kirill.shutemov@...ux.intel.com,
	hughd@...gle.com,
	aneesh.kumar@...nel.org,
	yang@...amperecomputing.com,
	peterx@...hat.com,
	broonie@...nel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	Dev Jain <dev.jain@....com>
Subject: Race condition observed between page migration and page fault handling on arm64 machines

I and Ryan had a discussion and we thought it would be best to get feedback
from the community.

The migration mm selftest currently fails on arm64 for shared anon mappings,
due to the following race:

Migration:						Page fault:
try_to_migrate_one():					handle_pte_fault():
1. Nuke the PTE						PTE has been deleted => do_pte_missing()
2. Mark the PTE for migration				PTE has not been deleted but is just not "present" => do_swap_page()

In the test, the parent thread migrates a single page between nodes, and the
children threads read on that page.

The race happens when the PTE has been nuked, and before it gets marked for
migration, the reader faults and checks if the PTE is missing, and calls
do_pte_missing() instead of the correct do_swap_page(); the latter implies that
the reader ends up calling migration_entry_wait() to wait for PTEs to get
corrected. The former path ends up following this: do_fault() -> do_read_fault()
-> __do_fault() -> vma->vm_ops->fault, which is shmem_fault() -> shmem_get_folio_gfp()
-> filemap_get_entry(), which then calls folio_try_get() and takes a reference
on the folio which is under migration. Returning back, the reader blocks on
folio_lock() since the migration path has the lock, and migration ends up
failing in folio_migrate_mapping(), which expects a reference count of 2 on the
folio.

The following hack makes the race vanish:

mm/rmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index e8fc5ecb59b2..bb10b3376595 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -2126,7 +2126,9 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
 				 * transition on a cached TLB entry is written through
 				 * and traps if the PTE is unmapped.
 				 */
-				pteval = ptep_get_and_clear(mm, address, pvmw.pte);
+				pteval = pte_mkinvalid(*(pvmw.pte));
+				*(pvmw.pte) = pteval;

 				set_tlb_ubc_flush_pending(mm, pteval, address);
 			} else {

It's a hack because the invalidation is non-atomic, and pte_mkinvalid() is
defined only on arm64. I could think of the following solutions:

1. Introduce (atomic)_pte_mkinvalid() for all arches and call the arch-API if
defined, else call ptep_get_and_clear(). The theoretical race would still exist
for other arches.

2. Prepare the migration swap entry and do an atomic exchange to fill the PTE
(this currently seems the best option to me, but I have not tried it out).

3. In try_to_migrate_one(), before the nuking, freeze the refcount of the folio.
This would solve the race, but then we will have to defer all the reference
releases till later, and I don't know whether that's correct or feasible.

4. Since the extra refcount being taken in filemap_get_entry() is due to loading
the folio from the pagecache, delete/invalidate the folio in the pagecache
until migration is complete. I tried with some helpers present in mm/filemap.c
to do that but I guess I am not aware of the subtleties, and I end up triggering
a BUG() somewhere.

5. In do_fault(), under the if block, we are already checking whether this is
just a temporary clearing of the PTE. We can take that out of the if block, but
then that would be a performance bottleneck since we are taking the PTL?

Thanks,
Dev