linux-kernel - Re: [PATCH v1 0/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

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: <7d88ac63-46b9-4b2e-a46b-c78d8d1d9f0e@redhat.com>
Date: Fri, 26 Jul 2024 11:19:15 +0200
From: David Hildenbrand <david@...hat.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 Muchun Song <muchun.song@...ux.dev>, Peter Xu <peterx@...hat.com>,
 Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH v1 0/2] mm/hugetlb: fix hugetlb vs. core-mm PT locking

On 25.07.24 22:41, Andrew Morton wrote:
> On Thu, 25 Jul 2024 20:39:53 +0200 David Hildenbrand <david@...hat.com> wrote:
> 
>> Working on another generic page table walker that tries to avoid
>> special-casing hugetlb, I found a page table locking issue with hugetlb
>> folios that are not mapped using a single PMD/PUD.
>>
>> For some hugetlb folio sizes, GUP will take different page table locks
>> when walking the page tables than hugetlb when modifying the page tables.
>>
>> I did not actually try reproducing an issue, but looking at
>> follow_pmd_mask() where we might be rereading a PMD value multiple times
>> it's rather clear that concurrent modifications are rather unpleasant.
>>
>> In follow_page_pte() we might be better in that regard -- ptep_get() does
>> a READ_ONCE() -- but who knows what else could happen concurrently in
>> some weird corner cases (e.g., hugetlb folio getting unmapped and freed).
>>
>> Did some basic sanity testing with various hugetlb sizes on x86-64 and
>> arm64. Maybe I'll find some time to actually write a simple reproducer in
>> the common weeks, so this wouldn't have to be all-theoretical for now.
> 
> When can we be confident that this change is merge-worthy?

I'm convinced that it is the right thing to do, but I don't think we 
have to rush this.

As Baolin notes, we fixed the same issue in the past, unfortunately also 
without a reproducer IIUC, so I'll try to reproduce the race, but I'm 
not 100% sure if I'll manage to do so..

So it's certainly merge-worthy after it had a bit of exposure to -next, 
but no need to rush this upstream.

Thanks!

-- 
Cheers,

David / dhildenb