linux-kernel - Re: [PATCH 2/5] mm,hugetlb: Document the reason to lock the folio in the faulting path

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1297fdd5-3de2-45bc-b146-e14061643fee@redhat.com>
Date: Mon, 16 Jun 2025 16:41:20 +0200
From: David Hildenbrand <david@...hat.com>
To: Oscar Salvador <osalvador@...e.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 Muchun Song <muchun.song@...ux.dev>, James Houghton <jthoughton@...gle.com>,
 Peter Xu <peterx@...hat.com>, Gavin Guo <gavinguo@...lia.com>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] mm,hugetlb: Document the reason to lock the folio in
 the faulting path

On 16.06.25 16:10, Oscar Salvador wrote:
> On Mon, Jun 16, 2025 at 11:22:43AM +0200, David Hildenbrand wrote:
>   
>> hugetlb_fault->hugetlb_no_page->hugetlb_wp
>>
>> already *mapped* the pagecache page into the page table.
>>
>> See
>> 	if (anon_rmap)
>> 		hugetlb_add_new_anon_rmap(folio, vma, vmf->address);
>> 	else
>> 		hugetlb_add_file_rmap(folio);
>>
>> So at that point it would be "stable" unless I am missing something?
>>
>> So once we are in hugetlb_wp(), that path much rather corresponds to
>> do_wp_page()->wp_page_copy.
> 
> Yes, that's right.
> That's something I've been thinking over the weekend.
> 
> E.g: do_cow_fault, first copies the page from the pagecache to a new one
> and __then__ maps the that page into the page tables.
> While in hugetlb_no_page->hugetlb_wp, the workflow is a bit different.
> 
> We first map it and then we copy it if we need to.
> 
> What do you mean by stable?

The same "stable" you used in the doc, that I complained about ;)

> In the generic faulting path, we're not worried about the page going away
> because we hold a reference, so I guess the lock must be to keep content stable?

What you want to avoid is IIRC, is someone doing a truncation/reclaim on 
the folio while you are mapping it.

Take a look at truncate_inode_pages_range() where we do a folio_lock() 
around truncate_inode_folio().

In other words, while you hold the folio lock (and verified that the 
folio was not truncated yet: for example, that folio->mapping is still 
set), you know that it cannot get truncated concurrently -- without 
holding other expensive locks.

Observe how truncate_cleanup_folio() calls

	if (folio_mapped(folio))
		unmap_mapping_folio(folio);

To remove all page table mappings.

So while holding the folio lock, new page table mappings are not 
expected to appear (IIRC).

> 
> I mean, yes, after we have mapped the page privately into the pagetables,
> we don't have business about content-integrity anymore, so given this rule, yes,
> I guess hugetlb_wp() wouldn't need the lock (for !anonymous) because we already
> have mapped it privately at that point.

That's my understanding. And while holding the PTL it cannot get 
unmapped. Whenever you temporarily drop the PTL, you have to do a 
pte_same() check to make sure concurrent truncation didn't happen.

So far my understanding at least of common filemap code.

> 
> But there's something I don't fully understand and makes me feel uneasy.
> If the lock in the generic faultin path is to keep content stable till we
> have mapped it privately, wouldn't be more correct to also hold it
> during the copy in hugetlb_wp, to kinda emulate that?
As long there us a page table mapping, it cannot get truncated. So if 
you find a PTE under PTL that maps that folio, truncation could not have 
happened.

-- 
Cheers,

David / dhildenb