linux-kernel - Re: [PATCH v1 1/2] mm/page_alloc: conditionally split > pageblock_order pages in free_one_page() and move_freepages_block

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: <882cb1b3-8519-46b7-b902-38a4f751b247@suse.cz>
Date: Mon, 9 Dec 2024 22:36:40 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Zi Yan <ziy@...dia.com>
Cc: David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
 Johannes Weiner <hannes@...xchg.org>, linux-mm@...ck.org,
 Andrew Morton <akpm@...ux-foundation.org>, Yu Zhao <yuzhao@...gle.com>
Subject: Re: [PATCH v1 1/2] mm/page_alloc: conditionally split >
 pageblock_order pages in free_one_page() and move_freepages_block_isolate()

On 12/9/24 20:23, Zi Yan wrote:
> On 9 Dec 2024, at 14:01, Vlastimil Babka wrote:
>>> +	/*
>>> +	 * With CONFIG_MEMORY_ISOLATION, we might be freeing MAX_ORDER_NR_PAGES
>>> +	 * pages that cover pageblocks with different migratetypes; for example
>>> +	 * only some migratetypes might be MIGRATE_ISOLATE. In that (unlikely)
>>> +	 * case, fallback to freeing individual pageblocks so they get put
>>> +	 * onto the right lists.
>>> +	 */
>>> +	if (!IS_ENABLED(CONFIG_MEMORY_ISOLATION) ||
>>> +	    likely(order <= pageblock_order) ||
>>> +	    pfnblock_migratetype_equal(pfn + pageblock_nr_pages, end_pfn, mt)) {
>>> +		__free_one_page(page, pfn, zone, order, mt, fpi_flags);
>>> +		return;
>>> +	}
>>>
>>> -		__free_one_page(page, pfn, zone, order, mt, fpi);
>>> -		pfn += 1 << order;
>>> +	while (pfn != end_pfn) {
>>> +		mt = get_pfnblock_migratetype(page, pfn);
>>> +		__free_one_page(page, pfn, zone, pageblock_order, mt, fpi_flags);
>>> +		pfn += pageblock_nr_pages;
>>>  		page = pfn_to_page(pfn);
>>
>> This predates your patch, but seems potentially dangerous to attempt
>> pfn_to_page(end_pfn) with SPARSEMEM and no vmemmap and the end_pfn perhaps
>> being just outside of the valid range? Should we change that?
>>
>> But seems this code was initially introduced as part of Johannes'
>> migratetype hygiene series.
> 
> It starts as split_free_page() from commit b2c9e2fbba32 ("mm: make
> alloc_contig_range work at pageblock granularity”), but harmless since
> it is only used to split a buddy page. Then commit fd919a85cd55 ("mm:
> page_isolation: prepare for hygienic freelists") refactored it, which
> should be fine, since it is still used for the same purpose in page
> isolation. Then commit e98337d11bbd ("mm/contig_alloc: support __GFP_COMP")
> used it for gigantic hugetlb.
> 
> For SPARSEMEM && !SPARSEMEM_VMEMMAP, PFNs are contiguous, vmemmap might not
> be. The code above using pfn in the loop might be fine. And since order
> is provided, unless the caller is providing a falsely large order, pfn
> should be valid. Or am I missing anything?

I mean if we are in the last iteration and about to exit the loop because
pfn == end_pfn, and it's the very last MAX_ORDER block of a zone and
section, end_pfn is already outside of it, and pfn_to_page() might get NULL
result from __pfn_to_section() and __section_mem_map_addr() then oops, no?

> Best Regards,
> Yan, Zi