Re: [RFC PATCH v2 04/25] iommu/dma: Add PISA block allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm: page_alloc: PISA hooks bypass gfp_allowed_mask and
  current_gfp_context()
- [Medium] iommu/dma: reclaim lock inversion on pisa_meta_mutex in PISA
  page allocation
- [Low] iommu/pisa: KUnit test_alloc_and_recycle() fails when
  SKB_FRAG_PAGE_ORDER is 0
--

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

iommu/dma: Add PISA block allocation

This commit adds PISA block allocation and compound page splitting, along
with __GFP_PISA_RO and __GFP_PISA_RW flags hooked into the page allocator.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index cf559ee86d6f9..1f1b99023094c 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c
[ ... ]
> +/* Obtain and split a new PISA page on @nid into blocks of @order. */
> +static struct pisa_meta *pisa_alloc_split_page(unsigned int order, gfp_t gfp, int nid)
> +{
> +	bool can_block = gfpflags_allow_blocking(gfp), clean;
> +	unsigned int bpp = PISA_BLOCKS(order);
> +	struct pisa_meta *meta;
> +	struct page *page;
> +
> +	/* __GFP_DMA/__GFP_DMA32 were already declined by pisa_alloc_bulk(). */
> +	meta = pisa_reservoir_pop(nid, gfp & ~__GFP_ZERO, &clean);

[Severity: Medium]
Can a GFP_NOFS or GFP_NOIO allocation invert lock ordering on
pisa_meta_mutex when the reservoir misses?

pisa_meta_init() and pisa_meta_memory_cb() hold pisa_meta_mutex across
GFP_KERNEL allocations, establishing pisa_meta_mutex -> fs_reclaim.  Here,
GFP_NOFS and GFP_NOIO still have __GFP_DIRECT_RECLAIM set, so
pisa_reservoir_pop() -> __pisa_page_alloc() calls pisa_meta_ensure_pfn()
with can_block == true while the caller holds filesystem or I/O locks:

drivers/iommu/pisa-meta.c:pisa_meta_ensure_pfn() {
    ...
	if (can_block && !pisa_meta_pte((unsigned long)pisa_meta_array + (idx << PAGE_SHIFT))) {
		int err;

		mutex_lock(&pisa_meta_mutex);
		err = pisa_meta_prealloc(pisa_meta_array, pisa_meta_nframes, pfn, pfn + 1);
		mutex_unlock(&pisa_meta_mutex);
    ...
}

CPU0                              CPU1
----                              ----
lock(fs_lock)
                                  mutex_lock(&pisa_meta_mutex)
                                  apply_to_page_range()
                                    /* fs_reclaim */
                                    lock(fs_lock)
pisa_alloc_split_page(GFP_NOFS)
  ...
    pisa_meta_ensure_pfn(..., true)
      mutex_lock(&pisa_meta_mutex)

 *** DEADLOCK ***

[ ... ]

> diff --git a/drivers/iommu/pisa-kunit.c b/drivers/iommu/pisa-kunit.c
> index 079f6220e742a..bdc52e6497b05 100644
> --- a/drivers/iommu/pisa-kunit.c
> +++ b/drivers/iommu/pisa-kunit.c
> @@ -87,9 +90,78 @@ static void test_meta_active_toggle(struct kunit *test)
>  
>  static void test_alloc_and_recycle(struct kunit *test)
>  {
> -	KUNIT_EXPECT_NULL(test, pisa_create(NUMA_NO_NODE, 0, DMA_NONE));
> -	KUNIT_EXPECT_NULL(test, pisa_create(NUMA_NO_NODE, PISA_ORDER + 1, DMA_BIDIRECTIONAL));
> +	const struct {
> +		unsigned int order;
> +		bool ro;
> +	} cases[] = {
> +		{ 0, false },
> +		{ 2, true },
> +		{ SKB_FRAG_PAGE_ORDER, true },
> +	};
> +	struct page *p1, *p2, *bulk[3];
> +	struct pisa_alloc *pa;
> +	unsigned int i;
> +
> +	KUNIT_EXPECT_NULL(test, pisa_create(first_memory_node, 0, DMA_NONE));
> +	KUNIT_EXPECT_NULL(test, pisa_create(first_memory_node, PISA_ORDER + 1,
> +					    DMA_BIDIRECTIONAL));
> +	KUNIT_EXPECT_NULL(test, pisa_node_pa(first_memory_node, 1, false));
> +	KUNIT_EXPECT_NULL(test, pisa_node_pa(first_memory_node, 0, true));

[Severity: Low]
Doesn't this KUNIT_EXPECT_NULL(test, pisa_node_pa(first_memory_node, 0,
true)) check fail on arm64 with CONFIG_ARM64_64K_PAGES=y?

With 64KB pages, SKB_FRAG_PAGE_ORDER is get_order(32768) == 0, so
pisa_init() populates pisa_nodes[nid].pa[true][0] for the
{ SKB_FRAG_PAGE_ORDER, true } entry that this test also asserts is
non-NULL in the loop below.

[ ... ]

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 48df87a19d312..b8e1f0fcb3f41 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
[ ... ]
> @@ -5223,6 +5246,14 @@ unsigned long alloc_pages_bulk_noprof(gfp_t gfp, int preferred_nid,
>  	while (nr_populated < nr_pages && page_array[nr_populated])
>  		nr_populated++;
>  
> +	if (unlikely(gfp & __GFP_PISA_RO)) {
> +		nr_populated = pisa_alloc_pages_bulk(gfp, 0, preferred_nid, nodemask,
> +						     nr_pages, page_array, false);
> +		gfp &= ~__GFP_PISA_RW;
> +		if (nr_populated == nr_pages)
> +			goto out;
> +	}
> +
>  	/* No pages requested? */
>  	if (unlikely(nr_pages <= 0))
>  		goto out;
> @@ -5457,6 +5488,13 @@ struct page *__alloc_frozen_pages_noprof(gfp_t gfp, unsigned int order,
>  		fastpath_alloc_flags |= ALLOC_WMARK_LOW;
>  	}
>  
> +	if (unlikely(gfp & __GFP_PISA_RO)) {
> +		page = pisa_alloc_pages(gfp, order, preferred_nid, nodemask);
> +		if (page)
> +			return page;
> +		gfp &= ~__GFP_PISA_RW;
> +	}
> +
>  	gfp &= gfp_allowed_mask;
>  	/*
>  	 * Apply scoped allocation constraints. This is mainly about GFP_NOFS

[Severity: Medium]
Should both PISA hooks run only after gfp &= gfp_allowed_mask and
gfp = current_gfp_context(gfp)?

Callers inside a memalloc_nofs_save() or memalloc_noio_save() scope (or
during system suspend when pm_restrict_gfp_mask() clears __GFP_IO |
__GFP_FS in gfp_allowed_mask) pass GFP_KERNEL | __GFP_PISA_* and rely on
the page allocator to strip __GFP_FS and __GFP_IO.  Passing the raw gfp
into pisa_alloc_pages_bulk() and pisa_alloc_pages() lets
__pisa_page_alloc() see (__GFP_FS | __GFP_IO) still set:

drivers/iommu/pisa-node.c:__pisa_page_alloc() {
	if (unlikely(!static_branch_unlikely(&pisa_key))) {
		if (!can_block || (gfp & (__GFP_FS | __GFP_IO)) != (__GFP_FS | __GFP_IO) ||
		    pisa_meta_init())
			return ERR_PTR(-EAGAIN);
	}
    ...
	if (can_block)
		memflags = pisa_memalloc_save(gfp);
}

As a result, __pisa_page_alloc() can invoke pisa_meta_init() inside a
NOFS, NOIO, or suspend context, and pisa_memalloc_save(gfp) fails to set
PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS when gfp_allowed_mask has restricted
I/O or filesystem reclaim.

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