linux-kernel - Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <865621ac-81e4-5396-ded1-3502b1e5a061@redhat.com>
Date:   Tue, 11 Jan 2022 10:00:36 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Alistair Popple <apopple@...dia.com>, Peter Xu <peterx@...hat.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Cc:     Andrea Arcangeli <aarcange@...hat.com>,
        Yang Shi <shy828301@...il.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Kirill A . Shutemov" <kirill@...temov.name>
Subject: Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details
 specified

On 11.01.22 08:40, Alistair Popple wrote:
> On Monday, 10 January 2022 7:37:15 PM AEDT David Hildenbrand wrote:
>> On 15.11.21 14:49, Peter Xu wrote:
>>> This check existed since the 1st git commit of Linux repository, but at that
>>> time there's no page migration yet so I think it's okay.
>>>
>>> With page migration enabled, it should logically be possible that we zap some
>>> shmem pages during migration.  When that happens, IIUC the old code could have
>>> the RSS counter accounted wrong on MM_SHMEMPAGES because we will zap the ptes
>>> without decreasing the counters for the migrating entries.  I have no unit test
>>> to prove it as I don't know an easy way to trigger this condition, though.
>>>
>>> Besides, the optimization itself is already confusing IMHO to me in a few points:
>>>
>>>   - The wording "skip swap entries" is confusing, because we're not skipping all
>>>     swap entries - we handle device private/exclusive pages before that.
>>
>> I think one part of the confusion is "swap vs non-swap" entries.
>> For !pte_none() && !pte_present() we can have
>>
>> * swap entry
>> * non-swap entry
>> ** device exclusive entry
>> ** device private entry
>> ** HWpoison entry
>> ** migration entry
>>
>> So the comment claims to skip "swap entries" but also skips HWpoison and
>> migration entries, and I think that's the confusing part.
>> Both only apply to PageAnon().
> 
> I must be missing something but why do these only apply to PageAnon()?

My memory might be wrong. I remember that for PageAnon() we need
migration/hwpoison entries because there is no way we could refault the
page from a mapping once we zap the entry. For everything else, we could
zap and refault. But looks like we indeed also use migration/hwpoison
entries for pages with a mapping, although it might not be strictly
required.

> 
>> IIUC, the only way we could get details != NULL is via unmap_mapping_page()+unmap_mapping_pages().
>>
>> I do wonder if any of the callers really cares about PageAnon() pages where this would be relevant.
>>
>> Am I wrong or is unmap_mapping_pages() never called with "even_cows == true" and we can remove
>> that paremeter:
> 
> Except that unmap_mapping_range() takes `even_cows` as a parameter and passes
> that to unmap_mapping_pages(), and from what I can tell there are callers of
> unmap_mapping_range() that set `even_cows = true`.

You're right.

-- 
Thanks,

David / dhildenb