linux-kernel - Re: [PATCH 1/2] mm,memory_hotplug: Remove un-taken lock

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: <0506ae4e-e17d-4c3c-aa3e-1cea04909e5a@redhat.com>
Date: Wed, 22 May 2024 17:24:17 +0200
From: David Hildenbrand <david@...hat.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: Oscar Salvador <osalvador@...e.de>,
 Andrew Morton <akpm@...ux-foundation.org>, Mike Rapoport <rppt@...nel.org>,
 Michal Hocko <mhocko@...e.com>, Anshuman Khandual
 <anshuman.khandual@....com>, Vlastimil Babka <vbabka@...e.cz>,
 Pavel Tatashin <pasha.tatashin@...een.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mm,memory_hotplug: Remove un-taken lock

On 22.05.24 16:27, Brendan Jackman wrote:
> On Wed, May 22, 2024 at 04:09:41PM +0200, David Hildenbrand wrote:
>> On 21.05.24 14:57, Brendan Jackman wrote:
>> The old seqlock guaranteed that we would have obtained consistent values
>> here. start + spanned_pages defines a range. For example, growing a zone to
>> the beginning implies that both ranges must be changed.
>>
>> I do wonder if it might be better to instead have zone->zone_start_pfn and
>> zone->zone_end_pfn. That way, both can be changed individually, not
>> requiring adjustment of both to grow/shrink a zone at the beginning.
> 
> Thanks this is a good point.
> 
> So basically the fact that spanned_pages is "once or eventually"
> correct is certainly not enough because it only has meaning with
> reference to zone_start_pfn. I didn't realise this because of my
> spontaneous inspiration to believe that zone_start_pfn was fixed.

Right, it isn't.

> 
> By the way, some noob questions: am I OK with my assumption that it's
> fine for reader code to operate on zone spans that are both stale and
> "from the future"? thinking abstractly I guess that seeing a stale
> value when racing with offline_pages is roughly the same as seeing a
> value "from the future" when racing with online_pages?

Right. PFN walkers should be using pfn_to_online_page(), where races are 
possible but barely seen in practice.

zone handlers like mm/compaction.c can likely deal with races, although 
it might all be cleaner (and safer?) when using start+end. I recall it 
also recalls on pfn_to_online_page().

Regarding page_outside_zone_boundaries(), it should be fine if we can 
read start+end atomically, that way we would not accidentally report 
"page outside ..." when changing the start address. I think with your 
current patch that might happen (although likely extremely hard to 
trigger) when growing the zone at the start, reducing zone_start_pfn.

> 
> Also, is it ever possible for pages to get removed and then added back
> and end up in a different zone than before?

Yes. Changing between MOVABLE and NORMAL is possible and can easily be 
triggered by offlining+re-onlining memory blocks.

-- 
Cheers,

David / dhildenb