linux-kernel - Re: [PATCH 09/10] mm: page_isolation: prepare for hygienic freelists

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: <419F2502-507B-4228-860B-217EE74C45B6@nvidia.com>
Date: Thu, 21 Mar 2024 11:03:58 -0400
From: Zi Yan <ziy@...dia.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: kernel test robot <lkp@...el.com>,
 Andrew Morton <akpm@...ux-foundation.org>, oe-kbuild-all@...ts.linux.dev,
 Linux Memory Management List <linux-mm@...ck.org>,
 Vlastimil Babka <vbabka@...e.cz>, Mel Gorman <mgorman@...e.de>,
 "\"Huang, Ying\"" <ying.huang@...el.com>,
 David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/10] mm: page_isolation: prepare for hygienic freelists

On 21 Mar 2024, at 10:24, Johannes Weiner wrote:

> On Thu, Mar 21, 2024 at 09:13:57PM +0800, kernel test robot wrote:
>> Hi Johannes,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on akpm-mm/mm-everything]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Johannes-Weiner/mm-page_alloc-remove-pcppage-migratetype-caching/20240321-020814
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
>> patch link:    https://lore.kernel.org/r/20240320180429.678181-10-hannes%40cmpxchg.org
>> patch subject: [PATCH 09/10] mm: page_isolation: prepare for hygienic freelists
>> config: i386-randconfig-003-20240321 (https://download.01.org/0day-ci/archive/20240321/202403212118.ye7lcKjD-lkp@intel.com/config)
>> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/202403212118.ye7lcKjD-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@...el.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202403212118.ye7lcKjD-lkp@intel.com/
>>
>> All warnings (new ones prefixed by >>):
>>
>>    mm/page_alloc.c: In function 'move_freepages_block_isolate':
>>>> mm/page_alloc.c:688:17: warning: array subscript 11 is above array bounds of 'struct free_area[11]' [-Warray-bounds]
>>      688 |  zone->free_area[order].nr_free--;
>>          |  ~~~~~~~~~~~~~~~^~~~~~~
>>>> mm/page_alloc.c:688:17: warning: array subscript 11 is above array bounds of 'struct free_area[11]' [-Warray-bounds]
>
> I think this is a bug in the old gcc.
>
> We have this in move_freepages_block_isolate():
>
> 	/* We're the starting block of a larger buddy */
> 	if (PageBuddy(page) && buddy_order(page) > pageblock_order) {
> 		int mt = get_pfnblock_migratetype(page, pfn);
> 		int order = buddy_order(page);
>
> 		if (!is_migrate_isolate(mt))
> 			__mod_zone_freepage_state(zone, -(1UL << order), mt);
> 		del_page_from_free_list(page, zone, order);
>
> And this config doesn't have hugetlb enabled, so:
>
> /* If huge pages are not used, group by MAX_ORDER_NR_PAGES */
> #define pageblock_order         MAX_PAGE_ORDER
>
> If buddies were indeed >MAX_PAGE_ORDER, this would be an out-of-bounds
> access when delete updates the freelist count. Of course, buddies per
> definition cannot be larger than MAX_PAGE_ORDER. But the older gcc
> doesn't seem to realize this branch in this configuration is dead.
>
> Maybe we can help it out and make the impossible scenario a bit more
> explicit? Does this fixlet silence the warning?
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index efb2581ac142..4cdc356e73f6 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1698,6 +1698,10 @@ bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>  				       NULL, NULL))
>  		return false;
>
> +	/* No splits needed if buddies can't span multiple blocks */
> +	if (pageblock_order == MAX_PAGE_ORDER)
> +		goto move;
> +
>  	/* We're a tail block in a larger buddy */
>  	pfn = find_large_buddy(start_pfn);
>  	if (pfn != start_pfn) {
> @@ -1725,7 +1729,7 @@ bool move_freepages_block_isolate(struct zone *zone, struct page *page,
>  		split_large_buddy(zone, page, pfn, order);
>  		return true;
>  	}
> -
> +move:
>  	mt = get_pfnblock_migratetype(page, start_pfn);
>  	nr_moved = move_freepages(zone, start_pfn, end_pfn, migratetype);
>  	if (!is_migrate_isolate(mt))
>
> Zi Yan, does this look sane to you as well?
Yes. This and patch 9 look good to me. Thanks.

For both, Reviewed-by: Zi Yan <ziy@...dia.com>

I also tested them locally and confirm it is a gcc bug and this fix
works for gcc-9.3:
1. gcc-13.2 does not give any warning for the original patch 9
2. gcc-9.3 gives the warning for the origin patch, but the warning goes
away with this patch applied.

--
Best Regards,
Yan, Zi

Download attachment "signature.asc" of type "application/pgp-signature" (855 bytes)