Re: amusing SLUB compaction bug when CC_OPTIMIZE_FOR_SIZE

From: Hyeonggon Yoo
Date: Tue Oct 25 2022 - 09:47:58 EST

On Mon, Oct 24, 2022 at 04:35:04PM +0200, Vlastimil Babka wrote:

[,,,]

> I would like to have a working safe version in -next, even if we are able
> simplify it later thanks to frozen refcounts. I've made a formal patch of
> yours, but I'm still convinced the slab check needs to be more paranoid so
> it can't observe a false positive __folio_test_movable() while missing the
> folio_test_slab(), hence I added the barriers as in my previous attempt [1].
> Does that work for you and can I add your S-o-b?
>
> [1] https://lore.kernel.org/all/aec59f53-0e53-1736-5932-25407125d4d4@xxxxxxx/
>
> ----8<----
> From 1d481f279f07d332ea381dfd6247a292ad403ed6 Mon Sep 17 00:00:00 2001
> From: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> 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@xxxxxxx: 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@xxxxxxxxx/
>
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
> mm/migrate.c | 38 ++++++++++++++++++++++++++------------
> mm/slab.c | 6 +++++-
> mm/slub.c | 6 +++++-
> 3 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1379e1912772..ad79e7c23db5 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,29 @@ 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;
> +
> + if (unlikely(folio_test_slab(folio)))
> + goto out_put;
> + /* Pairs with smp_wmb() in slab freeing, e.g. SLUB's __free_slab() */
> + smp_rmb();
> /*
> - * 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.
> */
> - 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;
> +
> /*
> * As movable pages are not isolated from LRU lists, concurrent
> * compaction threads can race against page migration functions
> @@ -92,8 +106,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 +121,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 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?
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()
mops->isolate_page() (*crash*)


Please let me know if I'm missing something ;-)
Thanks!

--
Hyeonggon