linux-kernel - Re: [PATCH] mm/huge_memory: fix dereferencing invalid pmd migration entry

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eec1be5d-8b42-6dbb-432d-488650b79c40@google.com>
Date: Wed, 16 Apr 2025 22:03:33 -0700 (PDT)
From: Hugh Dickins <hughd@...gle.com>
To: Zi Yan <ziy@...dia.com>
cc: Gavin Guo <gavinguo@...lia.com>, linux-mm@...ck.org, 
    akpm@...ux-foundation.org, willy@...radead.org, linmiaohe@...wei.com, 
    hughd@...gle.com, revest@...gle.com, kernel-dev@...lia.com, 
    linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
    Naoya Horiguchi <n-horiguchi@...jp.nec.com>
Subject: Re: [PATCH] mm/huge_memory: fix dereferencing invalid pmd migration
 entry

On Mon, 14 Apr 2025, Zi Yan wrote:
> On 14 Apr 2025, at 3:27, Gavin Guo wrote:
> 
> > When migrating a THP, concurrent access to the PMD migration entry
> > during a deferred split scan can lead to a page fault, as illustrated
> 
> It is an access violation, right? Because pmd_folio(*pmd_migration_entry)
> does not return a folio address. Page fault made this sounded like not
> a big issue.
> 
> > below. To prevent this page fault, it is necessary to check the PMD
> > migration entry and return early. In this context, there is no need to
> > use pmd_to_swp_entry and pfn_swap_entry_to_page to verify the equality
> > of the target folio. Since the PMD migration entry is locked, it cannot
> > be served as the target.
> 
> You mean split_huge_pmd_address() locks the PMD page table, so that
> page migration cannot proceed, or the THP is locked by migration,
> so that it cannot be split? The sentence is a little confusing to me.

No, split_huge_pmd_address() locks nothing. But its caller holds the
folio lock on this folio (as split_huge_pmd_locked() asserts with a 
VM_WARN_ON_ONCE); and page migration holds folio lock on its folio
(as various swapops.h functions assert with BUG_ON).

So any PMD migration entry found here cannot be for the folio which
split_huge_pmd_address() is passing down.  (And even if the impossible
did occur, what woud we want to do?  Skip it as the patch does.)

> 
> >
> > BUG: unable to handle page fault for address: ffffea60001db008
> > CPU: 0 UID: 0 PID: 2199114 Comm: tee Not tainted 6.14.0+ #4 NONE
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> > RIP: 0010:split_huge_pmd_locked+0x3b5/0x2b60
> > Call Trace:
> > <TASK>
> > try_to_migrate_one+0x28c/0x3730
> > rmap_walk_anon+0x4f6/0x770
> > unmap_folio+0x196/0x1f0
> > split_huge_page_to_list_to_order+0x9f6/0x1560
> > deferred_split_scan+0xac5/0x12a0
> > shrinker_debugfs_scan_write+0x376/0x470
> > full_proxy_write+0x15c/0x220
> > vfs_write+0x2fc/0xcb0
> > ksys_write+0x146/0x250
> > do_syscall_64+0x6a/0x120
> > entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >
> > The bug is found by syzkaller on an internal kernel, then confirmed on
> > upstream.
> >
> > Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Gavin Guo <gavinguo@...lia.com>
> > ---
> >  mm/huge_memory.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 2a47682d1ab7..0cb9547dcff2 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -3075,6 +3075,8 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >  void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >  			   pmd_t *pmd, bool freeze, struct folio *folio)
> >  {
> > +	bool pmd_migration = is_pmd_migration_entry(*pmd);
> > +
> >  	VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
> >  	VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
> >  	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> > @@ -3085,10 +3087,18 @@ void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >  	 * require a folio to check the PMD against. Otherwise, there
> >  	 * is a risk of replacing the wrong folio.
> >  	 */
> > -	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> > -	    is_pmd_migration_entry(*pmd)) {
> > -		if (folio && folio != pmd_folio(*pmd))
> > -			return;
> > +	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || pmd_migration) {
> > +		if (folio) {
> > +			/*
> > +			 * Do not apply pmd_folio() to a migration entry; and
> > +			 * folio lock guarantees that it must be of the wrong
> > +			 * folio anyway.
> 
> Why does the folio lock imply it is a wrong folio?

Because you cannot have two tasks holding folio lock on the same folio
at the same time.  So therefore it is a different ("wrong") folio.

> 
> > +			 */
> > +			if (pmd_migration)
> > +				return;
> > +			if (folio != pmd_folio(*pmd))
> > +				return;
> > +		}
> 
> Why not just
> 
> if (folio && pmd_migration)
> 	return;

That looks nicer, less indentation, I agree.  But Gavin's patch is
keeping the relevant check next to the "pmd_folio(*pmd)" to be avoided:
also good. I have no opinion which is the better.

Hugh

> 
> if (pmd_trans_huge() …) {
> 	…
> }
> ?
> 
> Thanks.
> 
> Best Regards,
> Yan, Zi
>