linux-kernel - Re: [PATCH v1 2/2] mm/hugetlb: document why hugetlb uses folio

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

Message-ID: <9e9ac4a3-461f-4ada-a41c-4432c947c818@redhat.com>
Date: Thu, 2 May 2024 16:33:39 +0200
From: David Hildenbrand <david@...hat.com>
To: Peter Xu <peterx@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 linux-kselftest@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
 Muchun Song <muchun.song@...ux.dev>, Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH v1 2/2] mm/hugetlb: document why hugetlb uses
 folio_mapcount() for COW reuse decisions

On 02.05.24 16:28, Peter Xu wrote:
> On Thu, May 02, 2024 at 10:52:59AM +0200, David Hildenbrand wrote:
>> Let's document why hugetlb still uses folio_mapcount() and is prone to
>> leaking memory between processes, for example using vmsplice() that
>> still uses FOLL_GET.
>>
>> More details can be found in [1], especially around how hugetlb pages
>> cannot really be overcommitted, and why we don't particularly care about
>> these vmsplice() leaks for hugetlb -- in contrast to ordinary memory.
>>
>> [1] https://lore.kernel.org/all/8b42a24d-caf0-46ef-9e15-0f88d47d2f21@redhat.com/
>>
>> Suggested-by: Peter Xu <peterx@...hat.com>
>> Signed-off-by: David Hildenbrand <david@...hat.com>
>> ---
>>   mm/hugetlb.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 417fc5cdb6eeb..a7efb350f5d07 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -5963,6 +5963,13 @@ static vm_fault_t hugetlb_wp(struct folio *pagecache_folio,
>>   	/*
>>   	 * If no-one else is actually using this page, we're the exclusive
>>   	 * owner and can reuse this page.
>> +	 *
>> +	 * Note that we don't rely on the (safer) folio refcount here, because
>> +	 * copying the hugetlb folio when there are unexpected (temporary)
>> +	 * folio references could harm simple fork()+exit() users when
>> +	 * we run out of free hugetlb folios: we would have to kill processes
>> +	 * in scenarios that used to work. As a side effect, there can still
>> +	 * be leaks between processes, for example, with FOLL_GET users.
>>   	 */
>>   	if (folio_mapcount(old_folio) == 1 && folio_test_anon(old_folio)) {
>>   		if (!PageAnonExclusive(&old_folio->page)) {
> 
> Thanks for preparing such updates, David.
> 
> However is fork+exit the real problem?  E.g. if a child simply fork, do
> something, then exit, I don't see a major issue even if we follow refcount
> here (despite the "check against 1 or 2 or 3" issue, where hugetlb_fault
> can take one already).  As long as the child quits, all ref / map counts
> will be released then.  If the child needs to write to ANON|PRIV it needs
> to manage hugetlb reservations anyways.

The PAE flag was cleared and if there is any unexpected (temporary) 
reference (page migration, lockless swapcache lookups, whatever), we're 
in trouble.

> 
> In the case of vmsplice it's kind of malicious, and holding that refcount
> (with 0 mapcount) doesn't sound the common scenario to me.

Yes, I'm not that concerned about something that that APP would be 
triggering (vmsplice).

> 
> IIUC if we need to keep this, it was more about the case where (as you
> correctly mentioned in another follow up reply) hugetlb isn't that flexible
> to memory overcommits, and in many cases it won't have extra pages floating
> around to allow adhoc CoWs?  While random refcount boost is easy to happen,
> and here the problem is we simply cannot identify that v.s. vmsplice()
> malicious takers.

Yes, exactly.

-- 
Cheers,

David / dhildenb