linux-kernel - Re: [merged] mm-compaction-abort-free-scanner-if-split-fails.patch removed from -mm tree

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: <aeb751a9-1ee3-d071-195f-db8775d93c00@suse.cz>
Date:	Wed, 29 Jun 2016 12:05:09 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Joonsoo Kim <iamjoonsoo.kim@....com>, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org
Cc:	rientjes@...gle.com, hughd@...gle.com, mgorman@...hsingularity.net,
	minchan@...nel.org, stable@...r.kernel.org,
	mm-commits@...r.kernel.org
Subject: Re: [merged] mm-compaction-abort-free-scanner-if-split-fails.patch
 removed from -mm tree

On 06/29/2016 10:12 AM, Joonsoo Kim wrote:
>> @@ -1035,8 +1034,12 @@ static void isolate_freepages(struct com
>>  			continue;
>>
>>  		/* Found a block suitable for isolating free pages from. */
>> -		isolate_freepages_block(cc, &isolate_start_pfn,
>> -					block_end_pfn, freelist, false);
>> +		isolated = isolate_freepages_block(cc, &isolate_start_pfn,
>> +						block_end_pfn, freelist, false);
>> +		/* If isolation failed early, do not continue needlessly */
>> +		if (!isolated && isolate_start_pfn < block_end_pfn &&
>> +		    cc->nr_migratepages > cc->nr_freepages)
>> +			break;
>
> Hello, David.
>
> Minchan found the bug on this patch.

Was it reported in some mail to linux-mm before the patch was sent to 
Linus, or just now?

> isolate_freepages_block() could return positive number if it is
> stopped due to memory shortage. In this case, above branch would not
> catch this situation due to positive 'isolated' and go through the

Uh, right. It could have isolated something successfully and only then 
fail to isolate some more, so isolated is non-zero. I've missed this 
corner case in review :(

> following code.
>
>   if ((cc->nr_freepages >= cc->nr_migratepages) || XXX)
>   else
>           VM_BUG_ON(isolate_start_pfn < block_end_pfn);
>
> In this case, cc->nr_freepages could be lower than cc->nr_migratepages
> and isolate_start_pfn < block_end_pfn so it triggers VM_BUG_ON().
>
> If my analysis is correct, please fix it.

David?

> Thanks.
>