Message-ID: <Y1koKX002Lc91qWL@hyeyoo>
Date:   Wed, 26 Oct 2022 21:29:29 +0900
From:   Hyeonggon Yoo <42.hyeyoo@...il.com>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Hugh Dickins <hughd@...gle.com>,
        David Laight <David.Laight@...lab.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        rcu@...r.kernel.org, "Paul E . McKenney" <paulmck@...nel.org>
Subject: Re: amusing SLUB compaction bug when CC_OPTIMIZE_FOR_SIZE

On Wed, Oct 26, 2022 at 12:52:01PM +0200, Vlastimil Babka wrote:
> On  10/25/22 16:08, Vlastimil Babka wrote:
> > On 10/25/22 15:47, Hyeonggon Yoo wrote:
> >> On Mon, Oct 24, 2022 at 04:35:04PM +0200, Vlastimil Babka wrote:
> >> 
> >> [,,,]
> >> 
> >>> diff --git a/mm/slab.c b/mm/slab.c
> >>> index 59c8e28f7b6a..219beb48588e 100644
> >>> --- a/mm/slab.c
> >>> +++ b/mm/slab.c
> >>> @@ -1370,6 +1370,8 @@ static struct slab *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
> >>>  
> >>>  	account_slab(slab, cachep->gfporder, cachep, flags);
> >>>  	__folio_set_slab(folio);
> >>> +	/* Make the flag visible before any changes to folio->mapping */
> >>> +	smp_wmb();
> >>>  	/* Record if ALLOC_NO_WATERMARKS was set when allocating the slab */
> >>>  	if (sk_memalloc_socks() && page_is_pfmemalloc(folio_page(folio, 0)))
> >>>  		slab_set_pfmemalloc(slab);
> >>> @@ -1387,9 +1389,11 @@ static void kmem_freepages(struct kmem_cache *cachep, struct slab *slab)
> >>>  
> >>>  	BUG_ON(!folio_test_slab(folio));
> >>>  	__slab_clear_pfmemalloc(slab);
> >>> -	__folio_clear_slab(folio);
> >>>  	page_mapcount_reset(folio_page(folio, 0));
> >>>  	folio->mapping = NULL;
> >>> +	/* Make the mapping reset visible before clearing the flag */
> >>> +	smp_wmb();
> >>> +	__folio_clear_slab(folio);
> >>>  
> >>>  	if (current->reclaim_state)
> >>>  		current->reclaim_state->reclaimed_slab += 1 << order;
> >>> diff --git a/mm/slub.c b/mm/slub.c
> >>> index 157527d7101b..6dc17cb915c5 100644
> >>> --- a/mm/slub.c
> >>> +++ b/mm/slub.c
> >>> @@ -1800,6 +1800,8 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node,
> >>>  
> >>>  	slab = folio_slab(folio);
> >>>  	__folio_set_slab(folio);
> >>> +	/* Make the flag visible before any changes to folio->mapping */
> >>> +	smp_wmb();
> >>>  	if (page_is_pfmemalloc(folio_page(folio, 0)))
> >>>  		slab_set_pfmemalloc(slab);
> >>>  
> >>> @@ -2008,8 +2010,10 @@ static void __free_slab(struct kmem_cache *s, struct slab *slab)
> >>>  	}
> >>>  
> >>>  	__slab_clear_pfmemalloc(slab);
> >>> -	__folio_clear_slab(folio);
> >>>  	folio->mapping = NULL;
> >>> +	/* Make the mapping reset visible before clearing the flag */
> >>> +	smp_wmb();
> >>> +	__folio_clear_slab(folio);
> >>>  	if (current->reclaim_state)
> >>>  		current->reclaim_state->reclaimed_slab += pages;
> >>>  	unaccount_slab(slab, order, s);
> >>> -- 
> >>> 2.38.0
> >> 
> >> Do we need to try this with memory barriers before frozen refcount lands in?
> > 
> > There was IIRC an unresolved issue with frozen refcount tripping the page
> > isolation code so I didn't want to be depending on that.

Understood.

> >> It's quite complicated and IIUC there is a still theoretical race:
> >> 
> >> At isolate_movable_page:        At slab alloc:                          At slab free:
> >>                                 folio = alloc_pages(flags, order)
> >> 
> >> folio_try_get()
> >> folio_test_slab() == false
> >>                                 __folio_set_slab(folio)
> >>                                 smp_wmb()
> >> 
> >>                                                                         call_rcu(&slab->rcu_head, rcu_free_slab);
> >> 
> >> 
> >> smp_rmb()
> >> __folio_test_movable() == true
> >> 
> >>                                                                         folio->mapping = NULL;
> >>                                                                         smp_wmb()
> >>                                                                         __folio_clear_slab(folio);
> >> smp_rmb()
> >> folio_test_slab() == false
> >> 
> >> folio_trylock()
> > 
> > There's also between above and below:
> > 
> > if (!PageMovable(page) || PageIsolated(page))
> > 	goto out_no_isolated;
> > 
> > mops = page_movable_ops(page);
> > 
> > If we put another smp_rmb() before the PageMovable test, could that have
> > helped? It would assure we observe the folio->mapping = NULL; from the "slab
> > free" side?
> > 
> > But yeah, it's getting ridiculous. Maybe there's a simpler way to check two
> > bits in two different bytes atomically. Or maybe it's just an impossible
> > task, I feel I just dunno computers at this point.
> 
> After more thought, I think I just made a mistake by doing two
> folio_test_slab() tests around a single __folio_test_movable(). What I was
> supposed to do was two __folio_test_movable() tests around a single
> folio_test_slab()... I hope. That should take care of your scenario, or do
> you see another one? Thanks.

I couldn't find one with this patch.
To best my understanding this looks at least correct to me.

Hope there is no other place that is confused
by anonymous checking for random page like here.

Thanks.

> ----8----
> From 5ca1c10f6411d73ad579b58d4fa10326bf77cf0a Mon Sep 17 00:00:00 2001
> From: Matthew Wilcox <willy@...radead.org>
> Date: Mon, 24 Oct 2022 16:11:27 +0200
> Subject: [PATCH] mm/migrate: make isolate_movable_page() skip slab pages
> 
> In the next commit we want to rearrange struct slab fields to allow a
> larger rcu_head. Afterwards, the page->mapping field will overlap
> with SLUB's "struct list_head slab_list", where the value of prev
> pointer can become LIST_POISON2, which is 0x122 + POISON_POINTER_DELTA.
> Unfortunately the bit 1 being set can confuse PageMovable() to be a
> false positive and cause a GPF as reported by lkp [1].
> 
> I think the real problem here is that isolate_movable_page() is
> insufficiently paranoid.  Looking at the gyrations that GUP and the
> page cache do to convince themselves that the page they got really is
> the page they wanted, there are a few missing pieces (eg checking that
> you actually got a refcount on _this_ page and not some random other
> page you were temporarily part of a compound page with).
> 
> This patch does three things:
> 
>  - Turns one of the comments into English.  There are some others
>    which I'm still scratching my head over.
>  - Uses a folio to help distinguish which operations are being done
>    to the head vs the specific page (this is somewhat an abuse of the
>    folio concept, but it's acceptable)
>  - Add the aforementioned check that we're actually operating on the
>    page that we think we want to be.
>  - Add a check that the folio isn't secretly a slab.
> 
> We could put the slab check in PageMapping and call it after taking
> the folio lock, but that seems pointless.  It's the acquisition of
> the refcount which stabilises the slab flag, not holding the lock.
> 
> [ vbabka@...e.cz: add memory barriers to SLAB and SLUB's page allocation
>   and freeing, and their counterparts to isolate_movable_page(), to make
>   the checks for folio_test_slab() and __folio_test_movable() SMP safe ]
> 
> [1] https://lore.kernel.org/all/208c1757-5edd-fd42-67d4-1940cc43b50f@intel.com/
> 
> Signed-off-by: Vlastimil Babka <vbabka@...e.cz>
> ---
>  mm/migrate.c | 40 ++++++++++++++++++++++++++++------------
>  mm/slab.c    |  6 +++++-
>  mm/slub.c    |  6 +++++-
>  3 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1379e1912772..f0f58e42c1d4 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -60,6 +60,7 @@
>  
>  int isolate_movable_page(struct page *page, isolate_mode_t mode)
>  {
> +	struct folio *folio = page_folio(page);
>  	const struct movable_operations *mops;
>  
>  	/*
> @@ -71,16 +72,31 @@ int isolate_movable_page(struct page *page, isolate_mode_t mode)
>  	 * the put_page() at the end of this block will take care of
>  	 * release this page, thus avoiding a nasty leakage.
>  	 */
> -	if (unlikely(!get_page_unless_zero(page)))
> +	if (unlikely(!folio_try_get(folio)))
>  		goto out;
>  
> +	/* Recheck the page is still part of the folio we just got */
> +	if (unlikely(page_folio(page) != folio))
> +		goto out_put;
> +
>  	/*
> -	 * Check PageMovable before holding a PG_lock because page's owner
> -	 * assumes anybody doesn't touch PG_lock of newly allocated page
> -	 * so unconditionally grabbing the lock ruins page's owner side.
> +	 * Check movable flag before taking the folio lock because
> +	 * we use non-atomic bitops on newly allocated page flags so
> +	 * unconditionally grabbing the lock ruins page's owner side.
> +	 * Make sure we don't have a slab folio here as its usage of the
> +	 * mapping field can cause a false positive movable flag.
>  	 */
> -	if (unlikely(!__PageMovable(page)))
> -		goto out_putpage;
> +	if (unlikely(!__folio_test_movable(folio)))
> +		goto out_put;
> +	/* Pairs with smp_wmb() in slab allocation, e.g. SLUB's alloc_slab_page() */
> +	smp_rmb();
> +	if (unlikely(folio_test_slab(folio)))
> +		goto out_put;
> +	/* Pairs with smp_wmb() in slab freeing, e.g. SLUB's __free_slab() */
> +	smp_rmb();
> +	if (unlikely(!__folio_test_movable(folio)))
> +		goto out_put;
> +
>  	/*
>  	 * As movable pages are not isolated from LRU lists, concurrent
>  	 * compaction threads can race against page migration functions
> @@ -92,8 +108,8 @@ int isolate_movable_page(struct page *page, isolate_mode_t mode)
>  	 * lets be sure we have the page lock
>  	 * before proceeding with the movable page isolation steps.
>  	 */
> -	if (unlikely(!trylock_page(page)))
> -		goto out_putpage;
> +	if (unlikely(!folio_trylock(folio)))
> +		goto out_put;
>  
>  	if (!PageMovable(page) || PageIsolated(page))
>  		goto out_no_isolated;
> @@ -107,14 +123,14 @@ int isolate_movable_page(struct page *page, isolate_mode_t mode)
>  	/* Driver shouldn't use PG_isolated bit of page->flags */
>  	WARN_ON_ONCE(PageIsolated(page));
>  	SetPageIsolated(page);
> -	unlock_page(page);
> +	folio_unlock(folio);
>  
>  	return 0;
>  
>  out_no_isolated:
> -	unlock_page(page);
> -out_putpage:
> -	put_page(page);
> +	folio_unlock(folio);
> +out_put:
> +	folio_put(folio);
>  out:
>  	return -EBUSY;
>  }
> diff --git a/mm/slab.c b/mm/slab.c
> index 59c8e28f7b6a..219beb48588e 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1370,6 +1370,8 @@ static struct slab *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
>  
>  	account_slab(slab, cachep->gfporder, cachep, flags);
>  	__folio_set_slab(folio);
> +	/* Make the flag visible before any changes to folio->mapping */
> +	smp_wmb();
>  	/* Record if ALLOC_NO_WATERMARKS was set when allocating the slab */
>  	if (sk_memalloc_socks() && page_is_pfmemalloc(folio_page(folio, 0)))
>  		slab_set_pfmemalloc(slab);
> @@ -1387,9 +1389,11 @@ static void kmem_freepages(struct kmem_cache *cachep, struct slab *slab)
>  
>  	BUG_ON(!folio_test_slab(folio));
>  	__slab_clear_pfmemalloc(slab);
> -	__folio_clear_slab(folio);
>  	page_mapcount_reset(folio_page(folio, 0));
>  	folio->mapping = NULL;
> +	/* Make the mapping reset visible before clearing the flag */
> +	smp_wmb();
> +	__folio_clear_slab(folio);
>  
>  	if (current->reclaim_state)
>  		current->reclaim_state->reclaimed_slab += 1 << order;
> diff --git a/mm/slub.c b/mm/slub.c
> index 99ba865afc4a..5e6519d5169c 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1800,6 +1800,8 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node,
>  
>  	slab = folio_slab(folio);
>  	__folio_set_slab(folio);
> +	/* Make the flag visible before any changes to folio->mapping */
> +	smp_wmb();
>  	if (page_is_pfmemalloc(folio_page(folio, 0)))
>  		slab_set_pfmemalloc(slab);
>  
> @@ -2000,8 +2002,10 @@ static void __free_slab(struct kmem_cache *s, struct slab *slab)
>  	int pages = 1 << order;
>  
>  	__slab_clear_pfmemalloc(slab);
> -	__folio_clear_slab(folio);
>  	folio->mapping = NULL;
> +	/* Make the mapping reset visible before clearing the flag */
> +	smp_wmb();
> +	__folio_clear_slab(folio);
>  	if (current->reclaim_state)
>  		current->reclaim_state->reclaimed_slab += pages;
>  	unaccount_slab(slab, order, s);
> -- 
> 2.38.0

Powered by blists - more mailing lists