Re: [RFC PATCH v2 10/25] iommu/dma: Use PISA for coherent DMA allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] iommu/pisa: unsafe list_for_each_entry() in
  pisa_alloc_coherent()
- [Medium] iommu/pisa: NULL deref in is_swiotlb_for_alloc() in KUnit
  test
- [Medium] iommu/dma: incomplete DMA range check in
  pisa_alloc_coherent()
- [Low] iommu/dma: uninitialized stack device in
  test_coherent_alloc_and_free()
--

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

iommu/dma: Use PISA for coherent DMA allocations

This commit serves coherent DMA allocations up to 2MB from the per-NUMA-node
order-0 read-write PISA allocator when dev->pisa_coherent is enabled.

> diff --git a/drivers/iommu/pisa-alloc.c b/drivers/iommu/pisa-alloc.c
> index b0d968aa9181c..25e4645310678 100644
> --- a/drivers/iommu/pisa-alloc.c
> +++ b/drivers/iommu/pisa-alloc.c
> @@ -10,6 +10,8 @@
>  #include <linux/bitops.h>
>  #include <linux/cache.h>
>  #include <linux/device.h>
> +#include <linux/dma-direct.h>
> +#include <linux/dma-map-ops.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/pisa.h>
>  #include <linux/export.h>
> @@ -697,6 +699,150 @@ unsigned long pisa_alloc_bulk(struct pisa_alloc *pa, gfp_t gfp,
>  	return i;
>  }
>  
> +/* Find and reserve @nblocks aligned contiguous blocks in @meta under @pa->lock. */
> +static struct page *pisa_take_contig(struct pisa_meta *meta, unsigned int nblocks,
> +				     unsigned long align_mask)
> +{
> +	DECLARE_BITMAP(busy, PISA_MAX_BLOCKS);
> +	struct pisa_alloc *pa = meta->pa;
> +	unsigned int bpp = PISA_BLOCKS(0), i;
> +	bool poisoned = false;
> +	unsigned long off;
> +	struct page *page;
> +
> +	if (pisa_meta_avail(meta) < nblocks)
> +		return NULL;
> +
> +	bitmap_or(busy, meta->clean_bitmap, meta->dirty_bitmap, bpp);
> +	bitmap_complement(busy, busy, bpp);
> +
> +	off = 0;
> +	for (;;) {
> +		off = bitmap_find_next_zero_area(busy, bpp, off, nblocks, align_mask);
> +		if (off >= bpp) {
> +			if (poisoned) {
> +				if (meta->on_list == PISA_LIST_IDLE_CLEAN ||
> +				    meta->on_list == PISA_LIST_IDLE_DIRTY)
> +					pa->num_idle_pages--;
> +				pisa_place_meta(meta);
> +			}
> +			return NULL;
> +		}
> +
> +		page = pfn_to_page(pisa_meta_to_pfn(meta) + off);
> +		for (i = 0; i < nblocks; i++) {
> +			if (!pisa_block_poisoned(pa, page + i))
> +				continue;
> +			if (__test_and_clear_bit(off + i, meta->clean_bitmap))
> +				meta->nr_clean--;
> +			else if (__test_and_clear_bit(off + i, meta->dirty_bitmap))
> +				meta->nr_dirty--;
> +			__set_bit(off + i, busy);
> +			poisoned = true;
> +			off += i + 1;
> +			break;
> +		}
> +		if (i == nblocks)
> +			break;
> +	}

[ ... ]

> +void *pisa_alloc_coherent(struct page **pagep, struct device *dev,
> +			  struct iommu_domain *domain, size_t size,
> +			  gfp_t gfp, unsigned long attrs)
> +{
> +	bool should_release = false;
> +	struct list_head *lists[3];
> +	unsigned long align_mask, flags;
> +	unsigned int nblocks, i;
> +	struct pisa_meta *meta;
> +	struct pisa_alloc *pa;
> +	struct page *page = NULL;
> +
> +	size = PAGE_ALIGN(size);
> +	if (!pagep || !dev || !READ_ONCE(dev->pisa_coherent) ||
> +	    !dev_is_dma_coherent(dev) || !size || size > PISA_SIZE)
> +		return NULL;
> +	if (min_not_zero(dev->coherent_dma_mask, dev->bus_dma_limit) <= DMA_BIT_MASK(32))
> +		return NULL;
> +	if ((current_gfp_context(gfp) & ~(__GFP_ZERO | __GFP_NOWARN)) != GFP_KERNEL ||
> +	    (attrs & ~DMA_ATTR_NO_WARN))
> +		return NULL;
> +
> +	if (domain) {
> +		struct pisa_window *win = pisa_dma_window(domain);
> +
> +		if (!win || pisa_window_declined(win) || !(domain->pgsize_bitmap & PISA_SIZE))
> +			return NULL;
> +	} else if (is_swiotlb_for_alloc(dev) ||
> +		   force_dma_unencrypted(dev) ||
> +		   !dma_coherent_ok(dev, PFN_PHYS(max_pfn) - 1, 1)) {
> +		return NULL;
> +	}
> +
> +	pa = pisa_node_pa(dev_to_node(dev), 0, false);
> +	if (!pa)
> +		return NULL;
> +
> +	nblocks = size >> PAGE_SHIFT;
> +	align_mask = (1UL << get_order(size)) - 1;
> +	lists[0] = &pa->partial;
> +	lists[1] = &pa->partial_dirty;
> +	lists[2] = &pa->idle;
> +
> +	spin_lock_irqsave(&pa->lock, flags);
> +	for (i = 0; i < ARRAY_SIZE(lists) && !page; i++) {
> +		list_for_each_entry(meta, lists[i], list) {
> +			page = pisa_take_contig(meta, nblocks, align_mask);
> +			if (page)
> +				break;
> +		}
> +	}

[Severity: Medium]
Can list_for_each_entry() walk off lists[i] onto a different list when
pisa_take_contig() encounters a poisoned block?

If pisa_take_contig() clears a poisoned block in meta and then fails to
find nblocks contiguous blocks (off >= bpp), it calls pisa_place_meta(meta)
before returning NULL.  pisa_place_meta() can move meta->list to another
list (for example from pa->idle to pa->partial, or from pa->partial to
pa->partial_dirty or pa->full).  Because pisa_take_contig() returns NULL,
list_for_each_entry() does not break; it follows meta->list.next on the new
list while testing for termination against lists[i], and then treats the
new list's head embedded in struct pisa_alloc as a struct pisa_meta.  Using
list_for_each_entry_safe() avoids following relinked nodes.

[ ... ]

> diff --git a/drivers/iommu/pisa-kunit.c b/drivers/iommu/pisa-kunit.c
> index ab64657ad5c58..72ee8986b4802 100644
> --- a/drivers/iommu/pisa-kunit.c
> +++ b/drivers/iommu/pisa-kunit.c
> @@ -352,6 +352,72 @@ static void test_map_and_domain_lifecycle(struct kunit *test)
>  }
>  #endif /* CONFIG_IOMMU_DMA */
>  
> +static void test_coherent_alloc_and_free(struct kunit *test)
> +{
> +	struct device dev = { .init_name = "pisa_kunit", .numa_node = NUMA_NO_NODE,
> +			      .coherent_dma_mask = DMA_BIT_MASK(64),
> +			      .pisa_coherent = true };

[Severity: Medium]
Does passing this stack-allocated struct device with a NULL domain to
pisa_alloc_coherent() cause a NULL pointer dereference when
CONFIG_DMA_RESTRICTED_POOL is enabled?

Because dev is neither initialized via device_initialize() nor
swiotlb_dev_init(), dev.dma_io_tlb_mem is NULL.  When
test_coherent_alloc_and_free() later calls pisa_alloc_coherent() with
domain == NULL, pisa_alloc_coherent() calls is_swiotlb_for_alloc(dev),
which dereferences dev->dma_io_tlb_mem->for_alloc under
CONFIG_DMA_RESTRICTED_POOL=y.

[ ... ]

> +	/* Freeing p1 returns its 16 order-0 pages; next 64K alloc reuses them. */
> +	KUNIT_EXPECT_TRUE(test, pisa_free_coherent(p1, SZ_64K));
> +	v3 = pisa_alloc_coherent(&p3, &dev, NULL, SZ_64K, GFP_KERNEL, 0);
> +	KUNIT_ASSERT_NOT_NULL(test, v3);
> +	KUNIT_EXPECT_PTR_EQ(test, p3, p1);
> +
> +	KUNIT_EXPECT_TRUE(test, pisa_free_coherent(p2, SZ_64K));
> +	KUNIT_EXPECT_TRUE(test, pisa_free_coherent(p3, SZ_64K));
> +}

[Severity: Low]
In addition to the uninitialized dev.dma_io_tlb_mem above, does this call
with domain == NULL fail the test and leak p2 on Confidential Computing
guests?

When domain is NULL, pisa_alloc_coherent() rejects the allocation if
force_dma_unencrypted(&dev) is true, so v3 is NULL and
KUNIT_ASSERT_NOT_NULL(test, v3) aborts before pisa_free_coherent(p2, SZ_64K)
runs (until commit "iommu/dma: Support decrypted PISA pages" later in the
series).

[ ... ]

> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 25e9a86835fd3..11052b65e573b 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -244,6 +245,12 @@ void *dma_direct_alloc(struct device *dev, size_t size,
>  	if (attrs & DMA_ATTR_NO_WARN)
>  		gfp |= __GFP_NOWARN;
>  
> +	cpu_addr = pisa_alloc_coherent(&page, dev, NULL, size, gfp, attrs);
> +	if (cpu_addr) {
> +		*dma_handle = phys_to_dma_direct(dev, page_to_phys(page), false);
> +		return cpu_addr;
> +	}

[Severity: Medium]
Can dma_direct_alloc() return a valid cpu_addr with *dma_handle set to
DMA_MAPPING_ERROR (or an out-of-mask DMA address) when dev->dma_range_map
does not cover all of system RAM?

pisa_alloc_coherent() only checks dma_coherent_ok(dev, PFN_PHYS(max_pfn) - 1, 1)
before allocating, and dma_direct_alloc() does not check
dma_coherent_ok(dev, page_to_phys(page), size) on the allocated page.  If
dev->dma_range_map covers the top of RAM at max_pfn - 1 but has holes in
lower RAM (where !dma_direct_all_ram_mapped(dev) is true, as in
a409d9600959 ("dma-mapping: fix dma_addressing_limited() if dma_range_map
can't cover all system RAM")) or translates regions with different offsets,
pisa_alloc_coherent() can return a page outside dev->dma_range_map and
phys_to_dma_direct() will set *dma_handle to DMA_MAPPING_ERROR.

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