Re: [RFC PATCH v2 16/25] mm/slub: Add SLAB_CACHE_PISA_RO for PISA-backed slab caches

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] mm/slub: order clamping in calculate_sizes() breaks non-4K
  pages and slab->objects
- [Medium] iommu/pisa: pisa_alloc_folio() pollutes per-node allocator on
  __GFP_THISNODE
- [Medium] mm/slub: __GFP_PISA_RO leaks into alloc_slab_obj_exts() via
  OBJCGS_CLEAR_MASK
- [Medium] mm/slub: alloc_slab_page() and pisa_alloc_folio() bypass
  current_gfp_context()
- [Low] iommu/pisa: kmem_cache leak in test_slub_page_alloc() on
  allocation failure
--

commit 48ed47e1e79e98a68d2521a0d03cf1f63453e5e9
Author: Luigi Rizzo <lrizzo@google.com>

mm/slub: Add SLAB_CACHE_PISA_RO for PISA-backed slab caches

This commit adds the SLAB_CACHE_PISA_RO flag so SLUB caches can allocate
zeroed, read-only PISA folios via pisa_alloc_folio().

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index 4b61ec84ed6d1..3b5c49d143a7e 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c
> @@ -848,6 +848,33 @@ unsigned long pisa_alloc_pages_bulk(gfp_t gfp, unsigned int order, int preferred
>  	return pisa_alloc_bulk(pa, gfp & ~__GFP_PISA_RW, nr_pages, page_array, frozen);
>  }
>  
> +/**
> + * pisa_alloc_folio - Allocate a frozen folio for SLUB (SLAB_CACHE_PISA_RO)
> + * @gfp: GFP flags (must have __GFP_PISA_RW or __GFP_PISA_RO set)
> + * @order: Folio order
> + * @nid: Target NUMA node (or NUMA_NO_NODE)
> + *
> + * Drops __GFP_THISNODE before calling the per-node allocator and verifies
> + * node placement afterwards.
> + *
> + * Return: Frozen folio of @order, or NULL.
> + */
> +struct folio *pisa_alloc_folio(gfp_t gfp, unsigned int order, int nid)
> +{
> +	struct page *page;
> +
> +	page = pisa_alloc_pages(gfp & ~__GFP_THISNODE, order, nid, NULL);
> +	if (!page)
> +		return NULL;
> +	if (unlikely((gfp & __GFP_THISNODE) && nid != NUMA_NO_NODE &&
> +		     page_to_nid(page) != nid)) {
> +		page_ref_unfreeze(page, 1);
> +		__free_pages(page, order);
> +		return NULL;
> +	}
> +	return page_folio(page);
> +}

[Severity: Medium]
Can stripping __GFP_THISNODE here attach a remote 2MB PISA page to nid's
per-node PISA allocator?

When ___slab_alloc() tries the local node first, it passes
GFP_NOWAIT | __GFP_NOWARN | __GFP_THISNODE.  If nid's PISA allocator and
reservoir are empty, pisa_alloc_pages() reaches __pisa_page_alloc(), which
calls alloc_pages_node(nid, gfp, PISA_ORDER) without __GFP_THISNODE.  When
nid has no free order-9 buddy pages, alloc_pages_node() falls back to a
remote node, and __pisa_alloc_bulk() binds that remote 2MB page's
pisa_meta to nid's pisa_alloc and sets PISA_ACTIVE.

pisa_alloc_folio() then rejects the block because page_to_nid(page) != nid
and calls __free_pages(page, order), which intercepts PISA_ACTIVE in
__free_pages_prepare() -> pisa_free_page() and recycles the remote block
back into nid's pisa_alloc instead of freeing the 2MB page.  Subsequent
__GFP_THISNODE allocations on nid will repeatedly pop and reject blocks
from that remote 2MB page.

> diff --git a/drivers/iommu/pisa-kunit.c b/drivers/iommu/pisa-kunit.c
> index 40bf7801183da..9cc1328893ace 100644
> --- a/drivers/iommu/pisa-kunit.c
> +++ b/drivers/iommu/pisa-kunit.c
[ ... ]
> @@ -621,6 +622,23 @@ static void test_reservoir_and_scrubber(struct kunit *test)
>  	WRITE_ONCE(pisa_scrub_max, old_scrub);
>  }
>  
> +static void test_slub_page_alloc(struct kunit *test)
> +{
> +	struct kmem_cache *s;
> +	void *obj;
> +
> +	s = kmem_cache_create("pisa_kunit_slab", 1024, NULL,
> +			      SLAB_NO_MERGE | SLAB_CACHE_PISA_RO);
> +	KUNIT_ASSERT_NOT_NULL(test, s);
> +
> +	obj = kmem_cache_alloc(s, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, obj);
> +	KUNIT_EXPECT_TRUE(test, is_pisa_page(virt_to_page(obj)));
> +	kmem_cache_free(s, obj);
> +
> +	kmem_cache_destroy(s);
> +}

[Severity: Low]
Does KUNIT_ASSERT_NOT_NULL(test, obj) leak the kmem_cache s if
kmem_cache_alloc() fails?  KUNIT_ASSERT_NOT_NULL() aborts the test
immediately before reaching kmem_cache_destroy(s).

> diff --git a/mm/slub.c b/mm/slub.c
> index 45e077c7526c2..63609768f4e3f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
[ ... ]
> @@ -3378,6 +3379,8 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node,
>  	if (unlikely(!allow_spin))
>  		page = alloc_frozen_pages_nolock(0/* __GFP_COMP is implied */,
>  								  node, order);
> +	else if (unlikely(flags & __GFP_PISA_RO))
> +		page = (struct page *)pisa_alloc_folio(flags, order, node);
>  	else if (node == NUMA_NO_NODE)
>  		page = alloc_frozen_pages(flags, order);
>  	else

[Severity: Medium]
Does calling pisa_alloc_folio() directly bypass current_gfp_context()?

alloc_frozen_pages() and __alloc_frozen_pages() apply
current_gfp_context(gfp) in __alloc_frozen_pages_noprof() to strip
__GFP_DIRECT_RECLAIM, __GFP_FS, or __GFP_IO under scoped
memalloc_noreclaim_save(), memalloc_nofs_save(), or memalloc_noio_save()
sections.  Neither alloc_slab_page() nor pisa_alloc_folio() applies
current_gfp_context(flags), so a GFP_KERNEL allocation from a
SLAB_CACHE_PISA_RO cache inside memalloc_noreclaim_save() still has
gfpflags_allow_blocking(gfp) true and can sleep in pisa_alloc_split_page(),
and inside memalloc_nofs_save() or memalloc_noio_save() can enter
pisa_meta_init() from __pisa_page_alloc() before pisa_memalloc_save() is
called.

> @@ -3484,7 +3487,7 @@ static struct slab *allocate_slab(struct kmem_cache *s, gfp_t flags,
>  	gfp_t alloc_gfp;
>  	void *start;
>  
> -	flags &= gfp_allowed_mask;
> +	flags &= gfp_allowed_mask & ~__GFP_PISA_RW;
>  
>  	flags |= s->allocflags;

[Severity: Medium]
Does __GFP_PISA_RO in s->allocflags leak into internal slabobj_ext
allocations?

flags |= s->allocflags sets __GFP_PISA_RO in flags after ~__GFP_PISA_RW is
masked out, and allocate_slab() then passes flags to account_slab() ->
alloc_slab_obj_exts().  Because OBJCGS_CLEAR_MASK does not include
__GFP_PISA_RW:

mm/slub.c:
#define OBJCGS_CLEAR_MASK	(__GFP_DMA | __GFP_RECLAIMABLE | \
				__GFP_ACCOUNT | __GFP_NOFAIL | \
				__GFP_THISNODE | __GFP_COMP)

alloc_slab_obj_exts() (and alloc_empty_sheaf() when a caller passes
__GFP_PISA_RO) leaves __GFP_PISA_RO set on the internal kmalloc call.  If
the slabobj_ext array exceeds KMALLOC_MAX_CACHE_SIZE,
___kmalloc_large_node() allocates the SLUB metadata array from a PISA RO
page.  Adding __GFP_PISA_RW to OBJCGS_CLEAR_MASK would prevent this.

[ ... ]

> @@ -8182,6 +8189,11 @@ static int calculate_sizes(struct kmem_cache_args *args, struct kmem_cache *s)
>  	if (s->flags & SLAB_RECLAIM_ACCOUNT)
>  		s->allocflags |= __GFP_RECLAIMABLE;
>  
> +	if (s->flags & SLAB_CACHE_PISA_RO) {
> +		s->allocflags |= __GFP_PISA_RO | __GFP_ZERO;
> +		order = clamp_t(unsigned int, order, 2, PAGE_ALLOC_COSTLY_ORDER);
> +	}
> +
>  	/*
>  	 * For kmalloc caches we enable sheaves later by
>  	 * bootstrap_kmalloc_sheaves() to avoid recursion.

[Severity: High]
Does clamping order to [2, PAGE_ALLOC_COSTLY_ORDER] after
calculate_order(size) break on 16KB and 64KB page kernels?

In pisa_init(), read-only PISA allocators are only created for order 2 and
SKB_FRAG_PAGE_ORDER:

drivers/iommu/pisa-alloc.c:pisa_init() {
	const struct {
		u8 order;
		bool ro;
		bool pcp;
	} cfg[] = {
		{ 0, false, true },
		{ 2, true, false },
		{ SKB_FRAG_PAGE_ORDER, true, true },
	};

On ARM64 with 16KB or 64KB pages, SKB_FRAG_PAGE_ORDER is 1 or 0, so no
order-3 (PAGE_ALLOC_COSTLY_ORDER) read-only PISA allocator exists.  When
calculate_order(size) returns 3, pisa_alloc_folio() looks up
pisa_node_pa(nid, 3, true), which returns NULL, and allocate_slab() skips
the s->min fallback for SLAB_CACHE_PISA_RO, causing every allocation from
the cache to fail.

In addition, clamping order up to 2 runs after calculate_order(size) has
enforced MAX_OBJS_PER_PAGE (32767).  On 64KB pages with s->size == 8,
forcing order 2 produces 32768 objects, which fits in the 16-bit OO_MASK of
s->oo but truncates the 15-bit unsigned slab->objects bitfield to 0 in
allocate_slab(), corrupting freelist setup in build_slab_freelist() or
looping in ___slab_alloc().  Finally, if s->size exceeds
PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER when SLUB_DEBUG inflates metadata,
clamping order down makes oo_objects(s->oo) == 0 before
do_kmem_cache_create() can reach disable_higher_order_debug.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182533.3594984-1-lrizzo@google.com?part=16