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

Message-ID: <96ae32c7-8e49-4802-a9ed-c5428b3a1f14@redhat.com>
Date: Tue, 30 Jul 2024 21:49:54 +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>,
 Qi Zheng <zhengqi.arch@...edance.com>, Hugh Dickins <hughd@...gle.com>
Subject: Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

On 30.07.24 20:44, Peter Xu wrote:
> On Mon, Jul 29, 2024 at 07:46:26PM +0200, David Hildenbrand wrote:
>> I see what you mean but this is a very similar pattern as used in
>> collapse_pte_mapped_thp(), no? There we have
>>
>> start_pte = pte_offset_map_nolock(mm, pmd, haddr, &ptl);
>> ...
>> if (!pml)
>> 	spin_lock(ptl);
>> ...
>> pte_unmap(start_pte);
>> if (!pml)
>> 	spin_unlock(ptl);
>>
>>
>> Again, I don't have a strong opinion on this, but doing it more similar to
>> collapse_pte_mapped_thp() to obtain locks makes it clearer to me. But if I
>> am missing something obvious please shout and I'll change it.
> 
> Right.. I don't think that path can change the pte pgtable either, and
> there is even the line Hugh left showing it's impossible:
> 
> 	if (!start_pte)		/* mmap_lock + page lock should prevent this */
> 		goto abort;
> 
> I was thinking maybe the page lock is the critical one, irrelevant of mmap
> lock.
> 
> No strong opinion either.  Not sure whether Hugh has some thoughts.  But
> maybe if we stick with pte_offset_map_nolock() and if there'll be a repost
> anyway, we could add a similar comment like this one showing that the pte
> pgtable should be actually as stable as the ptlock.

I think this all deserves some future cleanups :)

... for the time being I'll have to drop this patch completely. I should 
have known that virt_to_page() does not work on kmap'ed pages, but part 
of me didn't want to believe it.

CONFIG_HIGHPTE wants to make my life challenging :)

-- 
Cheers,

David / dhildenb