| 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
| ||
|
Message-ID: <5318B339.6010000@codeaurora.org>
Date: Thu, 06 Mar 2014 09:41:13 -0800
From: Laura Abbott <lauraa@...eaurora.org>
To: Vlastimil Babka <vbabka@...e.cz>,
Andrew Morton <akpm@...ux-foundation.org>,
Mel Gorman <mgorman@...e.de>
CC: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH] mm/compaction: Break out of loop on !PageBuddy in isolate_freepages_block
On 3/6/2014 2:22 AM, Vlastimil Babka wrote:
> On 03/06/2014 03:26 AM, Laura Abbott wrote:
>> We received several reports of bad page state when freeing CMA pages
>> previously allocated with alloc_contig_range:
>>
>> <1>[ 1258.084111] BUG: Bad page state in process Binder_A pfn:63202
>> <1>[ 1258.089763] page:d21130b0 count:0 mapcount:1 mapping: (null)
>> index:0x7dfbf
>> <1>[ 1258.096109] page flags: 0x40080068(uptodate|lru|active|swapbacked)
>>
>> Based on the page state, it looks like the page was still in use. The
>> page
>> flags do not make sense for the use case though. Further debugging showed
>> that despite alloc_contig_range returning success, at least one page
>> in the
>> range still remained in the buddy allocator.
>>
>> There is an issue with isolate_freepages_block. In strict mode (which CMA
>> uses), if any pages in the range cannot be isolated,
>> isolate_freepages_block
>> should return failure 0. The current check keeps track of the total
>> number
>> of isolated pages and compares against the size of the range:
>>
>> if (strict && nr_strict_required > total_isolated)
>> total_isolated = 0;
>>
>> After taking the zone lock, if one of the pages in the range is not
>> in the buddy allocator, we continue through the loop and do not
>
>> increment total_isolated. If we end up over isolating by more than
>> one page (e.g. last since page needed is a higher order page), it
>> is not possible to detect that the page was skipped. The fix is to
>
> I found it hard to grasp this sentence at first. Perhaps something like
> "if in the last iteration of the loop we isolate more than one page
> (e.g. ...), the check for total_isolated may pass and we fail to detect
> that a page was skipped" would be better?
>
Yes, that sounds much better.
>> bail out if the loop immediately if we are in strict mode. There's
>> no benfit to continuing anyway since we need all pages to be
>> isolated.
>
> That looks sound , but I wonder if it makes sense to keep the
> nr_strict_required stuff after this change. The check could then simply
> use 'if (pfn < end_pfn)' the same way as isolate_freepages_range does,
> right?
>
I had that thought as well. I'll fix that up for v2 along with the rest
of your comments.
>> Signed-off-by: Laura Abbott <lauraa@...eaurora.org>
>> ---
>> mm/compaction.c | 25 +++++++++++++++++++------
>> 1 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index b48c525..3190cef 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -263,12 +263,21 @@ static unsigned long
>> isolate_freepages_block(struct compact_control *cc,
>> struct page *page = cursor;
>>
>> nr_scanned++;
>> - if (!pfn_valid_within(blockpfn))
>> - continue;
>> + if (!pfn_valid_within(blockpfn)) {
>> + if (strict)
>> + break;
>> + else
>> + continue;
>> + }
>> +
>> if (!valid_page)
>> valid_page = page;
>> - if (!PageBuddy(page))
>> - continue;
>> + if (!PageBuddy(page)) {
>> + if (strict)
>> + break;
>> + else
>> + continue;
>> + }
>>
>> /*
>> * The zone lock must be held to isolate freepages.
>> @@ -288,8 +297,12 @@ static unsigned long
>> isolate_freepages_block(struct compact_control *cc,
>> break;
>>
>> /* Recheck this is a buddy page under lock */
>> - if (!PageBuddy(page))
>> - continue;
>> + if (!PageBuddy(page)) {
>> + if (strict)
>> + break;
>> + else
>> + continue;
>> + }
>
> To avoid this triple if-else occurence, you could instead do a "goto
> isolate_failed;" and put the if-else under said label at the end of the
> loop, also allowing extra cleanup, something like this:
>
> @@ -298,8 +298,6 @@ static unsigned long isolate_freepages_block(struct
> compact_control *cc,
>
> /* Found a free page, break it into order-0 pages */
> isolated = split_free_page(page);
> - if (!isolated && strict)
> - break;
> total_isolated += isolated;
> for (i = 0; i < isolated; i++) {
> list_add(&page->lru, freelist);
> @@ -310,7 +308,13 @@ static unsigned long isolate_freepages_block(struct
> compact_control *cc,
> if (isolated) {
> blockpfn += isolated - 1;
> cursor += isolated - 1;
> + continue;
> }
> +isolate_fail:
> + if (strict)
> + break;
> + else
> + continue;
>
>
> Thanks,
> Vlastimil
>
>> /* Found a free page, break it into order-0 pages */
>> isolated = split_free_page(page);
>>
>
Thanks,
Laura
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists