Message-ID: <20140814220704.GB26367@optiplex.redhat.com>
Date:	Thu, 14 Aug 2014 19:07:05 -0300
From:	Rafael Aquini <aquini@...hat.com>
To:	ryabinin.a.a@...il.com
Cc:	koct9i@...il.com, sasha.levin@...cle.com,
	akpm@...ux-foundation.org, vbabka@...e.cz, rientjes@...gle.com,
	mgorman@...e.de, iamjoonsoo.kim@....com, davej@...hat.com,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	a.ryabinin@...sung.com, lwoodman@...hat.com, riel@...hat.com,
	jweiner@...hat.com
Subject: Re: mm: compaction: buffer overflow in isolate_migratepages_range

On Thu, Aug 14, 2014 at 06:43:50PM -0300, Rafael Aquini wrote:
> On Thu, Aug 14, 2014 at 10:07:40PM +0400, Andrey Ryabinin wrote:
> > We discussed this with Konstantin and he suggested a better solution for this.
> > If I understood him correctly the main idea was to store bit
> > identifying ballon page
> > in struct page (special value in _mapcount), so we won't need to check
> > mapping->flags.
> >
> 
> Here goes what I thought doing, following that suggestion of Konstantin and yours. (I didn't tested it yet)
> 
> Comments are welcomed.
> 
> Cheers,
> -- Rafael
> 
> ---- 8< ----
> From: Rafael Aquini <aquini@...hat.com>
> Subject: mm: balloon_compaction: enhance balloon_page_movable() checkpoint against races
> 
> While testing linux-next for the Kernel Address Sanitizer patchset (KASAN) 
> Sasha Levin reported a buffer overflow warning triggered for 
> isolate_migratepages_range(), which lated was discovered happening due to
> a condition where balloon_page_movable() raced against move_to_new_page(),
> while the later was copying the page->mapping of an anon page.
> 
> Because we can perform balloon_page_movable() in a lockless fashion at 
> isolate_migratepages_range(), the dicovered race has unveiled the scheme 
> actually used to spot ballooned pages among page blocks that checks for
> page_flags_cleared() and dereference page->mapping to check its mapping flags
> is weak and potentially prone to stumble across another similar conditions 
> in the future.
> 
> Following Konstantin Khlebnikov's and Andrey Ryabinin's suggestions,
> this patch replaces the old page->flags && mapping->flags checking scheme
> with a more simple and strong page->_mapcount read and compare value test.
> Similarly to what is done for PageBuddy() checks, BALLOON_PAGE_MAPCOUNT_VALUE
> is introduced here to mark balloon pages. This allows balloon_page_movable()
> to skip the proven troublesome dereference of page->mapping for flag checking
> while it goes on isolate_migratepages_range() lockless rounds.
> page->mapping dereference and flag-checking will be performed later, when
> all locks are held properly.
> 
> ---
>  include/linux/balloon_compaction.h | 61 +++++++++++---------------------------
>  mm/balloon_compaction.c            | 53 +++++++++++++++++----------------
>  2 files changed, 45 insertions(+), 69 deletions(-)
> 
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index 089743a..1409ccc 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -108,54 +108,29 @@ static inline void balloon_mapping_free(struct address_space *balloon_mapping)
>  }
>  
>  /*
> - * page_flags_cleared - helper to perform balloon @page ->flags tests.
> + * balloon_page_movable - identify balloon pages that can be moved by
> + *			  compaction / migration.
>   *
> - * As balloon pages are obtained from buddy and we do not play with page->flags
> - * at driver level (exception made when we get the page lock for compaction),
> - * we can safely identify a ballooned page by checking if the
> - * PAGE_FLAGS_CHECK_AT_PREP page->flags are all cleared.  This approach also
> - * helps us skip ballooned pages that are locked for compaction or release, thus
> - * mitigating their racy check at balloon_page_movable()
> + * BALLOON_PAGE_MAPCOUNT_VALUE must be <= -2 but better not too close to
> + * -2 so that an underflow of the page_mapcount() won't be mistaken
> + * for a genuine BALLOON_PAGE_MAPCOUNT_VALUE.
>   */
> -static inline bool page_flags_cleared(struct page *page)
> +#define BALLOON_PAGE_MAPCOUNT_VALUE (-256)
> +static inline bool balloon_page_movable(struct page *page)
>  {
> -	return !(page->flags & PAGE_FLAGS_CHECK_AT_PREP);
> +	return atomic_read(&page->_mapcount) == BALLOON_PAGE_MAPCOUNT_VALUE;
>  }
>  
> -/*
> - * __is_movable_balloon_page - helper to perform @page mapping->flags tests
> - */
> -static inline bool __is_movable_balloon_page(struct page *page)
> +static inline void __balloon_page_set(struct page *page)
>  {
> -	struct address_space *mapping = page->mapping;
> -	return mapping_balloon(mapping);
> +	VM_BUG_ON_PAGE(!atomic_read(&page->_mapcount) != -1, page);
> +	atomic_set(&page->_mapcount, BALLOON_PAGE_MAPCOUNT_VALUE);
>  }
>  
> -/*
> - * balloon_page_movable - test page->mapping->flags to identify balloon pages
> - *			  that can be moved by compaction/migration.
> - *
> - * This function is used at core compaction's page isolation scheme, therefore
> - * most pages exposed to it are not enlisted as balloon pages and so, to avoid
> - * undesired side effects like racing against __free_pages(), we cannot afford
> - * holding the page locked while testing page->mapping->flags here.
> - *
> - * As we might return false positives in the case of a balloon page being just
> - * released under us, the page->mapping->flags need to be re-tested later,
> - * under the proper page lock, at the functions that will be coping with the
> - * balloon page case.
> - */
> -static inline bool balloon_page_movable(struct page *page)
> +static inline void __balloon_page_clear(struct page *page)
>  {
> -	/*
> -	 * Before dereferencing and testing mapping->flags, let's make sure
> -	 * this is not a page that uses ->mapping in a different way
> -	 */
> -	if (page_flags_cleared(page) && !page_mapped(page) &&
> -	    page_count(page) == 1)
> -		return __is_movable_balloon_page(page);
> -
> -	return false;
> +	VM_BUG_ON_PAGE(!balloon_page_movable(page), page)
> +	atomic_set(&page->_mapcount, -1);
>  }
>  
>  /*
> @@ -170,10 +145,8 @@ static inline bool balloon_page_movable(struct page *page)
>   */
>  static inline bool isolated_balloon_page(struct page *page)
>  {
> -	/* Already isolated balloon pages, by default, have a raised refcount */
> -	if (page_flags_cleared(page) && !page_mapped(page) &&
> -	    page_count(page) >= 2)
> -		return __is_movable_balloon_page(page);
> +	if (balloon_page_movable && page_count(page) > 1)
> +		return true;
>  
>  	return false;
>  }
> @@ -193,6 +166,7 @@ static inline void balloon_page_insert(struct page *page,
>  				       struct list_head *head)
>  {
>  	page->mapping = mapping;
> +	__balloon_page_set(page);
>  	list_add(&page->lru, head);
>  }
>  
> @@ -207,6 +181,7 @@ static inline void balloon_page_insert(struct page *page,
>  static inline void balloon_page_delete(struct page *page)
>  {
>  	page->mapping = NULL;
> +	__balloon_page_clear(page);
>  	list_del(&page->lru);
>  }
>  
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index 6e45a50..1cfb254 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -220,33 +220,32 @@ bool balloon_page_isolate(struct page *page)
>  	 * raise its refcount preventing __free_pages() from doing its job
>  	 * the put_page() at the end of this block will take care of
>  	 * release this page, thus avoiding a nasty leakage.
> +	 *
> +	 * As balloon pages are not isolated from LRU lists, concurrent
> +	 * compaction threads can race against page migration functions
> +	 * as well as race against the balloon driver releasing a page.
> +	 * The aforementioned operations are done under the safety of
> +	 * page lock, so lets be sure we hold it before proceeding the
> +	 * isolations steps here.
>  	 */
> -	if (likely(get_page_unless_zero(page))) {
> +	if (get_page_unless_zero(page) && trylock_page(page)) {
Oops, I need to change this one, as we can left behind pages with raised
refcounts if get_page_unless_zero(page) but trylock_page(page) fails
here.


>  		/*
> -		 * As balloon pages are not isolated from LRU lists, concurrent
> -		 * compaction threads can race against page migration functions
> -		 * as well as race against the balloon driver releasing a page.
> -		 *
> -		 * In order to avoid having an already isolated balloon page
> -		 * being (wrongly) re-isolated while it is under migration,
> -		 * or to avoid attempting to isolate pages being released by
> -		 * the balloon driver, lets be sure we have the page lock
> -		 * before proceeding with the balloon page isolation steps.
> +		 * A ballooned page, by default, has just one refcount.
> +		 * Prevent concurrent compaction threads from isolating
> +		 * an already isolated balloon page by refcount check.
>  		 */
> -		if (likely(trylock_page(page))) {
> -			/*
> -			 * A ballooned page, by default, has just one refcount.
> -			 * Prevent concurrent compaction threads from isolating
> -			 * an already isolated balloon page by refcount check.
> -			 */
> -			if (__is_movable_balloon_page(page) &&
> -			    page_count(page) == 2) {
> +		if (balloon_page_movable(page) && page_count(page) == 2) {
> +			struct address_space *mapping = page_mapping(page);
> +			if (likely(mapping_balloon(mapping))) {
>  				__isolate_balloon_page(page);
>  				unlock_page(page);
>  				return true;
> +			} else {
> +				dump_page(page, "not movable balloon page");
> +				WARN_ON(1);
>  			}
> -			unlock_page(page);
>  		}
> +		unlock_page(page);
>  		put_page(page);
>  	}
>  	return false;
> @@ -255,19 +254,21 @@ bool balloon_page_isolate(struct page *page)
>  /* putback_lru_page() counterpart for a ballooned page */
>  void balloon_page_putback(struct page *page)
>  {
> +	struct address_space *mapping;
>  	/*
>  	 * 'lock_page()' stabilizes the page and prevents races against
>  	 * concurrent isolation threads attempting to re-isolate it.
>  	 */
>  	lock_page(page);
>  
> -	if (__is_movable_balloon_page(page)) {
> +	mapping = page_mapping(page);
> +	if (balloon_page_movable(page) && mapping_balloon(mapping)) {
>  		__putback_balloon_page(page);
>  		/* drop the extra ref count taken for page isolation */
>  		put_page(page);
>  	} else {
> -		WARN_ON(1);
>  		dump_page(page, "not movable balloon page");
> +		WARN_ON(1);
>  	}
>  	unlock_page(page);
>  }
> @@ -286,16 +287,16 @@ int balloon_page_migrate(struct page *newpage,
>  	 */
>  	BUG_ON(!trylock_page(newpage));
>  
> -	if (WARN_ON(!__is_movable_balloon_page(page))) {
> +	mapping = page_mapping(page);
> +	if (!(balloon_page_movable(page) && mapping_balloon(mapping))) {
>  		dump_page(page, "not movable balloon page");
> -		unlock_page(newpage);
> -		return rc;
> +		WARN_ON(1);
> +		goto out;
>  	}
>  
> -	mapping = page->mapping;
>  	if (mapping)
>  		rc = __migrate_balloon_page(mapping, newpage, page, mode);
> -
> +out:
>  	unlock_page(newpage);
>  	return rc;
>  }
> -- 
> 1.9.3
> 
--
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