linux-kernel - Re: [PATCH 02/11] mm: compaction: introduce isolate_{free,migrate}pages

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]

Date:	Mon, 12 Dec 2011 16:30:52 +0000
From:	Mel Gorman <mel@....ul.ie>
To:	Michal Nazarewicz <mina86@...a86.com>
Cc:	Marek Szyprowski <m.szyprowski@...sung.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-media@...r.kernel.org, linux-mm@...ck.org,
	linaro-mm-sig@...ts.linaro.org,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Russell King <linux@....linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Ankita Garg <ankita@...ibm.com>,
	Daniel Walker <dwalker@...eaurora.org>,
	Arnd Bergmann <arnd@...db.de>,
	Jesse Barker <jesse.barker@...aro.org>,
	Jonathan Corbet <corbet@....net>,
	Shariq Hasnain <shariq.hasnain@...aro.org>,
	Chunsang Jeong <chunsang.jeong@...aro.org>,
	Dave Hansen <dave@...ux.vnet.ibm.com>
Subject: Re: [PATCH 02/11] mm: compaction: introduce
 isolate_{free,migrate}pages_range().

On Mon, Dec 12, 2011 at 04:22:39PM +0100, Michal Nazarewicz wrote:
> > <SNIP>
> >
> >>+		if (!pfn_valid_within(pfn))
> >>+			goto skip;
> >
> >The flow of this function in general with gotos of skipped and next
> >is confusing in comparison to the existing function. For example,
> >if this PFN is not valid, and no freelist is provided, then we call
> >__free_page() on a PFN that is known to be invalid.
> >
> >>+		++nr_scanned;
> >>+
> >>+		if (!PageBuddy(page)) {
> >>+skip:
> >>+			if (freelist)
> >>+				goto next;
> >>+			for (; start < pfn; ++start)
> >>+				__free_page(pfn_to_page(pfn));
> >>+			return 0;
> >>+		}
> >
> >So if a PFN is valid and !PageBuddy and no freelist is provided, we
> >call __free_page() on it regardless of reference count. That does not
> >sound safe.
> 
> Sorry about that.  It's a bug in the code which was caught later on.  The
> code should read ???__free_page(pfn_to_page(start))???.
> 

That will call free on valid PFNs but why is it safe to call
__free_page() at all?  You say later that CMA requires that all
pages in the range be valid but if the pages are in use, that does
not mean that calling __free_page() is safe. I suspect you have not
seen a problem because the pages in the range were free as expected
and not in use because of MIGRATE_ISOLATE.

> >> 		/* Found a free page, break it into order-0 pages */
> >> 		isolated = split_free_page(page);
> >> 		total_isolated += isolated;
> >>-		for (i = 0; i < isolated; i++) {
> >>-			list_add(&page->lru, freelist);
> >>-			page++;
> >>+		if (freelist) {
> >>+			struct page *p = page;
> >>+			for (i = isolated; i; --i, ++p)
> >>+				list_add(&p->lru, freelist);
> >> 		}
> >>
> >>-		/* If a page was split, advance to the end of it */
> >>-		if (isolated) {
> >>-			blockpfn += isolated - 1;
> >>-			cursor += isolated - 1;
> >>-		}
> >>+next:
> >>+		pfn += isolated;
> >>+		page += isolated;
> >
> >The name isolated is now confusing because it can mean either
> >pages isolated or pages scanned depending on context. Your patch
> >appears to be doing a lot more than is necessary to convert
> >isolate_freepages_block into isolate_freepages_range and at this point,
> >it's unclear why you did that.
> 
> When CMA uses this function, it requires all pages in the range to be valid
> and free. (Both conditions should be met but you never know.) 

It seems racy but I guess you are depending on MIGRATE_ISOLATE to keep
things sane which is fine. However, I strongly suspect that if there
is a race and a page is in use, then you will need to retry the
migration step. Calling __free_page does not look right because
something still has a reference to the page.

> This change
> adds a second way isolate_freepages_range() works, which is when freelist is
> not specified, abort on invalid or non-free page, but continue as usual if
> freelist is provided.
> 

Ok, I think you should be able to do that by not calling split_free_page
or adding to the list if !freelist with a comment explaining why the
pages are left on the buddy lists for the caller to figure out. Bail if
a page-in-use is found and have the caller check that the return value
of isolate_freepages_block == end_pfn - start_pfn.

> I can try and restructure this function a bit so that there are fewer ???gotos???,
> but without the above change, CMA won't really be able to use it effectively
> (it would have to provide a freelist and then validate if pages on it are
> added in order).
> 

Please do and double check that __free_page logic too.

-- 
Mel Gorman
SUSE Labs
--
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/