linux-kernel - Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte

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: <bf2069ed-f2b8-49d4-baf0-dbd2189362f9@redhat.com>
Date: Fri, 26 Jul 2024 18:02:17 +0200
From: David Hildenbrand <david@...hat.com>
To: Peter Xu <peterx@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 Andrew Morton <akpm@...ux-foundation.org>,
 Muchun Song <muchun.song@...ux.dev>, Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

On 26.07.24 17:36, Peter Xu wrote:
> On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote:
>> pte_lockptr() is the only *_lockptr() function that doesn't consume
>> what would be expected: it consumes a pmd_t pointer instead of a pte_t
>> pointer.
>>
>> Let's change that. The two callers in pgtable-generic.c are easily
>> adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a
>> pte_offset_map_nolock() to obtain the lock, even though we won't actually
>> be traversing the page table.
>>
>> This makes the code more similar to the other variants and avoids other
>> hacks to make the new pte_lockptr() version happy. pte_lockptr() users
>> reside now only in  pgtable-generic.c.
>>
>> Maybe, using pte_offset_map_nolock() is the right thing to do because
>> the PTE table could have been removed in the meantime? At least it sounds
>> more future proof if we ever have other means of page table reclaim.
> 
> I think it can't change, because anyone who wants to race against this
> should try to take the pmd lock first (which was held already)?

That doesn't explain why it is safe for us to assume that after we took 
the PMD lock that the PMD actually still points at a completely empty 
page table. Likely it currently works by accident, because we only have 
a single such user that makes this assumption. It might certainly be a 
different once we asynchronously reclaim page tables.

But yes, the PMD cannot get modified while we hold the PMD lock, 
otherwise we'd be in trouble

> 
> I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
> nicer here, but only if my understanding is correct.

I really don't like open-coding that. Fortunately we were able to limit 
the use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c 
so far.

-- 
Cheers,

David / dhildenb