linux-kernel - Re: [PATCH] mm/sparsemem: pfn_to_page is not valid yet on SPARSEMEM

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: <c63452a4-6a97-8995-0060-65c65adcad78@redhat.com>
Date:   Thu, 6 Feb 2020 15:37:40 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Baoquan He <bhe@...hat.com>
Cc:     Wei Yang <richardw.yang@...ux.intel.com>,
        akpm@...ux-foundation.org, osalvador@...e.de,
        dan.j.williams@...el.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/sparsemem: pfn_to_page is not valid yet on SPARSEMEM

On 06.02.20 15:07, Baoquan He wrote:
> On 02/06/20 at 02:55pm, David Hildenbrand wrote:
>> On 06.02.20 14:50, Baoquan He wrote:
>>> On 02/06/20 at 02:28pm, David Hildenbrand wrote:
>>>> On 06.02.20 13:53, Wei Yang wrote:
>>>>> When we use SPARSEMEM instead of SPARSEMEM_VMEMMAP, pfn_to_page()
>>>>> doesn't work before sparse_init_one_section() is called. This leads to a
>>>>> crash when hotplug memory.
>>>>>
>>>>> We should use memmap as it did.
>>>>>
>>>>> Fixes: ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")
>>>>> Signed-off-by: Wei Yang <richardw.yang@...ux.intel.com>
>>>>> CC: Dan Williams <dan.j.williams@...el.com>
>>>>> ---
>>>>>  mm/sparse.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/sparse.c b/mm/sparse.c
>>>>> index 5a8599041a2a..2efb24ff8f96 100644
>>>>> --- a/mm/sparse.c
>>>>> +++ b/mm/sparse.c
>>>>> @@ -882,7 +882,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn,
>>>>>  	 * Poison uninitialized struct pages in order to catch invalid flags
>>>>>  	 * combinations.
>>>>>  	 */
>>>>> -	page_init_poison(pfn_to_page(start_pfn), sizeof(struct page) * nr_pages);
>>>>> +	page_init_poison(memmap, sizeof(struct page) * nr_pages);
>>>>
>>>> If you add sub-sections that don't fall onto the start of the section,
>>>>
>>>> pfn_to_page(start_pfn) != memmap
>>>>
>>>> and your patch would break that under SPARSEMEM_VMEMMAP if I am not wrong.
>>>
>>> It returns the pfn_to_page(pfn) from __populate_section_memmap() and
>>> assign to memmap in vmemmap case, how come it breaks anything. Correct
>>> me if I was wrong.
>>
>> I'm sorry, I can't follow :) Can you elaborate?
>>
>> Was your comment targeted at why the old code cannot be broken or why
>> this patch cannot be broken?
> 
> Sorry for the confusion :-) the latter. I mean the returned memmap has been
> at the pfn_to_page(start_pfn) in SPARSEMEM_VMEMMAP case.

Yeah, at least for SPARSEMEM_VMEMMAP it is indeed right. Thanks :)


Now, about SPARSEMEM:

populate_section_memmap() does not care about nr_pages and will allocate
a memmap for the whole section. So, whenever we add sub-sections to a
section, we allocate a new memmap for the whole section. And we do
overwrite the memmap pointer in our section. ( sparse_add_section() )

That makes me assume that sub-section hot-add under SPARSEMEM is either

a) never enabled and only works with SPARSEMEM_VMEMMAP
b) horribly broken

And I think a) applies (looking at pfn_section_valid()). Therefore, we
don't have to care about sub-section hot-add specifics (and I would be
broken already)

Acked-by: David Hildenbrand <david@...hat.com>

-- 
Thanks,

David / dhildenb