linux-kernel - Re: [PATCH 9/9] mm, page_alloc: optionally disable pcplists during page isolation

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: <a35c1f92-b56a-e6c4-9920-33f99850eb76@redhat.com>
Date:   Tue, 6 Oct 2020 10:40:23 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Michal Hocko <mhocko@...e.com>, Vlastimil Babka <vbabka@...e.cz>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Pavel Tatashin <pasha.tatashin@...een.com>,
        Oscar Salvador <osalvador@...e.de>,
        Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH 9/9] mm, page_alloc: optionally disable pcplists during
 page isolation

On 06.10.20 10:34, Michal Hocko wrote:
> On Tue 22-09-20 16:37:12, Vlastimil Babka wrote:
>> Page isolation can race with process freeing pages to pcplists in a way that
>> a page from isolated pageblock can end up on pcplist. This can be fixed by
>> repeated draining of pcplists, as done by patch "mm/memory_hotplug: drain
>> per-cpu pages again during memory offline" in [1].
>>
>> David and Michal would prefer that this race was closed in a way that callers
>> of page isolation who need stronger guarantees don't need to repeatedly drain.
>> David suggested disabling pcplists usage completely during page isolation,
>> instead of repeatedly draining them.
>>
>> To achieve this without adding special cases in alloc/free fastpath, we can use
>> the same approach as boot pagesets - when pcp->high is 0, any pcplist addition
>> will be immediately flushed.
>>
>> The race can thus be closed by setting pcp->high to 0 and draining pcplists
>> once, before calling start_isolate_page_range(). The draining will serialize
>> after processes that already disabled interrupts and read the old value of
>> pcp->high in free_unref_page_commit(), and processes that have not yet disabled
>> interrupts, will observe pcp->high == 0 when they are rescheduled, and skip
>> pcplists. This guarantees no stray pages on pcplists in zones where isolation
>> happens.
>>
>> This patch thus adds zone_pcplist_disable() and zone_pcplist_enable() functions
>> that page isolation users can call before start_isolate_page_range() and after
>> unisolating (or offlining) the isolated pages. A new zone->pcplist_disabled
>> atomic variable makes sure we disable only pcplists once and don't enable
>> them prematurely in case there are multiple users in parallel.
>>
>> We however have to avoid external updates to high and batch by taking
>> pcp_batch_high_lock. To allow multiple isolations in parallel, change this lock
>> from mutex to rwsem.
> 
> The overall idea makes sense. I just suspect you are over overcomplicating 
> the implementation a bit. Is there any reason that we cannot start with
> a really dumb implementation first. The only user of this functionality
> is the memory offlining and that is already strongly synchronized
> (mem_hotplug_begin) so a lot of trickery can be dropped here. Should we
> find a new user later on we can make the implementation finer grained
> but now it will not serve any purpose. So can we simply update pcp->high
> and drain all pcp in the given zone and wait for all remote pcp draining
> in zone_pcplist_enable and updte revert all that in zone_pcplist_enable.
> We can stick to the existing pcp_batch_high_lock.
> 
> What do you think?
> 

My two cents, we might want to make use of this in some cases of
alloc_contig_range() soon ("try hard mode"). So I'd love to see a
synchronized mechanism. However, that can be factored out into a
separate patch, so this patch gets significantly simpler.


-- 
Thanks,

David / dhildenb