Re: [RFC PATCH v2 07/25] iommu/dma: Release PISA domain mappings on domain teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] iommu/dma: list and RCU corruption in pisa_domain_quiesce()
  on quarantine
- [Medium] iommu/dma: data race on meta->flags in pisa_quarantine_meta()
  and release
- [Medium] iommu/dma: build failure in pisa_domain_quiesce() due to
  static symbols
- [Medium] iommu/dma: missing struct pisa_meta declaration in
  pisa-priv.h stubs
--

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

iommu/dma: Release PISA domain mappings on domain teardown

This commit adds pisa_domain_release() and pisa_domain_quiesce() to unmap
active PISA pages and release or quiesce domain mappings when an IOMMU
domain is torn down or its last DMA API user detaches, quarantining and
leaking any 2MB page whose iommu_unmap() fails.

> diff --git a/drivers/iommu/pisa-map.c b/drivers/iommu/pisa-map.c
> index 2e8d635dec34c..2ba977bb68734 100644
> --- a/drivers/iommu/pisa-map.c
> +++ b/drivers/iommu/pisa-map.c

[ ... ]

> +/* Report a short IOMMU unmap and mark @meta quarantined so it is never reused. */
> +static void pisa_quarantine_meta(struct pisa_meta *meta, struct iommu_domain *domain,
> +				 int idx, dma_addr_t iova, size_t unmapped)
> +{
> +	pr_err_ratelimited("pisa: unmap of IOVA %pad in domain %p (index %d) returned %zu, quarantining PISA page\n",
> +			   &iova, domain, idx, unmapped);
> +	WRITE_ONCE(meta->flags, meta->flags | PISA_QUARANTINED);
> +}

[ ... ]

> +static unsigned int pisa_unmap_domain_locked(struct pisa_meta *meta,
> +					     struct iommu_domain *domain,
> +					     int idx, dma_addr_t base,
> +					     struct iommu_iotlb_gather *gather)
> +{
> +	dma_addr_t iova = base + pisa_meta_win_offset(meta);
> +	size_t unmapped;
> +
> +	if (!test_and_clear_bit(idx, meta->domains_mapped))
> +		return 0;
> +
> +	unmapped = iommu_unmap_fast(domain, iova, PISA_SIZE, gather);
> +	if (unlikely(unmapped != PISA_SIZE))
> +		pisa_quarantine_meta(meta, domain, idx, iova, unmapped);
> +	return 1;
> +}

[Severity: Medium]
Can concurrent updates to meta->flags in pisa_quarantine_meta() and
pisa_release_page() clobber each other?

pisa_unmap_domain_locked() clears idx in meta->domains_mapped before
calling iommu_unmap_fast() and pisa_quarantine_meta(), so a concurrent
pisa_release_page() -> pisa_unmap_all() can see bit idx already cleared
and proceed to update meta->flags without synchronization:

CPU0                                  CPU1
pisa_domain_quiesce()                 pisa_release_page()
  pisa_unmap_domain_locked()
    test_and_clear_bit(idx, ...)
                                        pisa_unmap_all()
                                        flags = meta->flags
    iommu_unmap_fast() [short unmap]
    pisa_quarantine_meta()
      meta->flags |= PISA_QUARANTINED
                                        meta->flags = flags & ~PISA_ACTIVE
                                          <-- drops PISA_QUARANTINED

When PISA_QUARANTINED is lost, pisa_release_page_rcu() frees the IOVA slot
and returns the still-mapped 2MB page to the buddy allocator.  With the
opposite interleaving, pisa_quarantine_meta() restores PISA_ACTIVE on the
retired page.

> +/*
> + * Unmap all PISA leaf PTEs from a domain whose last DMA API user detached,
> + * keeping its window and domain index so mappings are re-installed on demand.
> + */
> +void pisa_domain_quiesce(struct iommu_domain *domain)
> +{
> +	struct iommu_iotlb_gather gather;
> +	struct pisa_window *win;
> +	unsigned int slot, done = 0;
> +	bool release = false;
> +	dma_addr_t base;
> +	int idx;
> +
> +	if (!static_branch_unlikely(&pisa_key))
> +		return;
> +	win = pisa_dma_window(domain);
> +	/* Pairs with smp_store_release() in pisa_window_assign(). */
> +	base = win ? smp_load_acquire(&win->base) : 0;
> +	if (!base)
> +		return;
> +	idx = win->domain_idx - PISA_IDX_FIRST;
> +
> +	iommu_iotlb_gather_init(&gather);
> +	/* Keep call_rcu()-freed pages from reaching the buddy allocator before IOTLB sync. */
> +	rcu_read_lock();
> +	for_each_set_bit(slot, pisa_slot_bitmap, pisa_nr_slots) {
> +		struct pisa_meta *meta = READ_ONCE(pisa_slot2meta[slot]);
> +		struct pisa_alloc *pa;
> +		unsigned long flags;
> +
> +		if (!meta || !test_bit(idx, meta->domains_mapped))
> +			continue;
> +		done += pisa_unmap_domain_locked(meta, domain, idx, base, &gather);
> +		if (likely(!(meta->flags & PISA_QUARANTINED)))
> +			continue;
> +		pa = READ_ONCE(meta->pa);
> +		if (!pa)
> +			continue;
> +		spin_lock_irqsave(&pa->lock, flags);
> +		if (meta->pa == pa) {
> +			if (meta->on_list == PISA_LIST_IDLE_CLEAN ||
> +			    meta->on_list == PISA_LIST_IDLE_DIRTY)
> +				pa->num_idle_pages--;
> +			release |= pisa_place_meta(meta);
> +		}
> +		spin_unlock_irqrestore(&pa->lock, flags);
> +	}
> +
> +	if (done)
> +		iommu_iotlb_sync(domain, &gather);
> +	rcu_read_unlock();
> +	if (release)
> +		pisa_schedule_reclaim();

[Severity: Medium]
Does pisa-map.o fail to build at this commit because PISA_LIST_IDLE_CLEAN,
PISA_LIST_IDLE_DIRTY, pisa_place_meta(), and pisa_schedule_reclaim() are
still static or local to drivers/iommu/pisa-alloc.c?

Those declarations are only moved to drivers/iommu/pisa-priv.h in a later
patch in the series, commit "block, mm, iommu/dma: Support PISA for disk
I/O and scrub migration", which breaks bisection when CONFIG_PISA and
CONFIG_IOMMU_DMA are enabled.

[Severity: Medium]
Can pisa_domain_quiesce() call pisa_place_meta() on an idle page that has
already been detached from pa for release, corrupting pisa_free_list or
meta->rcu?

When an idle page (avail == bpp) is detached onto pisa_free_list or
queued with call_rcu(), meta->pa remains non-NULL until
pisa_release_page_rcu() runs after the grace period, and its slot stays
set in pisa_slot_bitmap.  If pisa_domain_quiesce() sees PISA_QUARANTINED
on that page, meta->pa == pa is still true and pisa_place_meta() runs
llist_add(&meta->llnode, &pisa_free_list) on it:

CPU0                                  CPU1
pisa_reclaim_work_fn()                pisa_domain_quiesce()
  ...                                   rcu_read_lock()
  pisa_release_page(meta)
    call_rcu(&meta->rcu, ...)
                                        pisa_unmap_domain_locked()
                                          pisa_quarantine_meta()
                                        spin_lock_irqsave(&pa->lock, ...)
                                        if (meta->pa == pa)
                                          pisa_place_meta(meta)
                                            llist_add(&meta->llnode,
                                                      &pisa_free_list)
                                              <-- overwrites live meta->rcu

Because meta->llnode and meta->rcu share an anonymous union, adding
meta->llnode to pisa_free_list overwrites the active RCU head (or creates
a cycle in pisa_free_list if pisa_reclaim_work_fn() has not popped it
yet), and a second pisa_release_page() after meta->pa is cleared to NULL
hits the !meta->pa branch and frees the quarantined 2MB page to the buddy
allocator.

In addition, pisa_shrink_list() moves idle pages onto pisa_shrink_scan()'s
local release_list and decrements pa->num_idle_pages without setting
meta->on_list to PISA_LIST_NONE, so pisa_domain_quiesce() will also
decrement pa->num_idle_pages a second time and run list_del_init() on
meta->list while pisa_shrink_scan() walks release_list unlocked.

> diff --git a/drivers/iommu/pisa-priv.h b/drivers/iommu/pisa-priv.h
> index 3ef6e51402ceb..f97f69a13d1a9 100644
> --- a/drivers/iommu/pisa-priv.h
> +++ b/drivers/iommu/pisa-priv.h

[ ... ]

>  #define pisa_nr_slots 0U
>  static inline void pisa_slots_init(void) {}
>  static inline int pisa_slot_alloc(struct pisa_meta *meta) { return 0; }
>  static inline void pisa_slot_free(unsigned int slot) {}
> +static inline int pisa_unmap_all(struct pisa_meta *meta) { return 0; }

[Severity: Medium]
Does including pisa-priv.h in drivers/iommu/iommu.c trigger a
-Wvisibility warning on struct pisa_meta when CONFIG_PISA is disabled?

struct pisa_meta is only declared inside #ifdef CONFIG_PISA, so when
CONFIG_PISA=n the parameter lists of pisa_slot_alloc() and
pisa_unmap_all() declare a local struct pisa_meta type, failing the build
under CONFIG_WERROR=y.

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