linux-kernel - Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon

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]

Date:   Mon, 9 Oct 2023 12:03:19 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Suren Baghdasaryan <surenb@...gle.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Muchun Song <muchun.song@...ux.dev>
Subject: Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page()
 into wp_can_reuse_anon_folio()

On 03.10.23 19:05, Suren Baghdasaryan wrote:
> On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@...hat.com> wrote:
>>
>> Let's clean up do_wp_page() a bit, removing two labels and making it
>> a easier to read.
>>
>> wp_can_reuse_anon_folio() now only operates on the whole folio. Move the
>> SetPageAnonExclusive() out into do_wp_page(). No need to do this under
>> page lock -- the page table lock is sufficient.
>>
>> Signed-off-by: David Hildenbrand <david@...hat.com>
>> ---
>>   mm/memory.c | 88 +++++++++++++++++++++++++++--------------------------
>>   1 file changed, 45 insertions(+), 43 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 1f0e3317cbdd..512f6f05620e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3358,6 +3358,44 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
>>          return ret;
>>   }
>>

Sorry for the late response.

>> +static bool wp_can_reuse_anon_folio(struct folio *folio,
>> +                                   struct vm_area_struct *vma)
> 
> Since this function is calling folio_move_anon_rmap(), I would suggest
> changing its name to wp_reuse_anon_folio(). This would clarify that

folio_move_anon_rmap() is *not* the reuse part, it's just an rmap 
optimization. You could remove the call here and the whole thing would 
still work :) In fact, we can call folio_move_anon_rmap() whenever we 
sure the folio belongs to a single VMA only and we're holding the page 
lock. ... but we cannot always reuse a folio in that case because there 
might be GUP references from another process.

Reuse is
* Setting PageAnonExclusive
* Write fault: wunprotect the page -> wp_page_reuse()

I went a bit back and forth while cleaning that function up, but calling 
it wp_reuse_anon_folio() would end up being confusing with 
wp_page_reuse() called afterwards [we should probably rename that one to 
wp_page_wunprotect() independently]. So I prefer to leave the actual 
(sub)page reuse part in the caller.

> it's actually doing that operation instead of just checking if it's
> possible. That would also let us keep unconditional
> SetPageAnonExclusive() in it and do that under folio lock like it used
> to do (keeping rules simple). Other than that, it looks good to me.

I really want to avoid passing a "struct page" to that function; once 
we're dealing with PTE-mapped THP, the page might actually be a tail 
page of the folio.

Thanks!

-- 
Cheers,

David / dhildenb