Re: [RFC PATCH v2 23/25] block, mm, iommu/dma: Support PISA for disk I/O and scrub migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] iommu/pisa: double pisa_place_meta() and recycling in
  pisa_user_scrub_alloc()
- [High] iommu/dma: 2MB PISA pages mapped into untrusted devices in
  __pisa_map_phys()
- [Medium] block: broken PISA promotion and bio repin in
  bio_upgrade_pages_to_pisa()
- [Medium] iommu/pisa: scrub worker never scheduled for sparse user_pa
  pages
- [Medium] iommu/pisa: early scrub loop exit and age reset in
  pisa_user_scrub_node()
- [Medium] iommu/pisa: block pisa_tx_bufs depends on
  net_pisa_tx_bufs_set() and CONFIG_NET
- [Medium] iommu/pisa: __pisa_bdev_enabled() misses gendisk sysfs and
  NVMe multipath
- [Medium] iommu/pisa: direction permission mismatch in
  __pisa_filemap_alloc() and pisa_zc_iter()
--

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

block, mm, iommu/dma: Support PISA for disk I/O and scrub migration

This commit extends the PISA user and page-cache allocators to support
O_DIRECT and buffered disk I/O with per-direction per-node allocators,
bypasses IOVA allocation for PISA pages in dma_iova_try_alloc(), and adds
background scrub migration for sparse user PISA pages.

> diff --git a/block/bio.c b/block/bio.c
> index b0fcc69de6535..5b43430b5280c 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -6,6 +6,7 @@
>  #include <linux/swap.h>
>  #include <linux/bio-integrity.h>
>  #include <linux/blkdev.h>
> +#include <linux/pisa.h>
>  #include <linux/uio.h>
>  #include <linux/iocontext.h>
>  #include <linux/slab.h>
> @@ -1316,6 +1317,43 @@ static inline bool bio_iov_bvec_aligned(const struct bio *bio,
>  }
>  #endif
>  
> +/*
> + * Migrate pinned order-0 user pages in @bio into PISA memory when enabled on
> + * @bio->bi_bdev; returns true if pages were released and the caller must retry.
> + */
> +static bool bio_upgrade_pages_to_pisa(struct bio *bio)
> +{
> +	enum dma_data_direction dir;
> +	struct page *pages[64];
> +	unsigned int i, nr = 0;
> +
> +	if (!IS_ENABLED(CONFIG_PISA) || !bio_flagged(bio, BIO_PAGE_PINNED))
> +		return false;
> +	dir = op_is_write(bio_op(bio)) ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +	if (!is_bdev_pisa_enabled(bio->bi_bdev, dir))
> +		return false;
> +
> +	for (i = 0; i < bio->bi_vcnt && nr < ARRAY_SIZE(pages); i++) {
> +		struct page *page = bio->bi_io_vec[i].bv_page;
> +
> +		if (bvec_nr_pages(&bio->bi_io_vec[i]) == 1 && !is_pisa_page(page)) {
> +			get_page(page);
> +			pages[nr++] = page;
> +		}
> +	}
> +	if (!nr)
> +		return false;
> +
> +	bio_release_pages(bio, false);
> +	bio->bi_vcnt = 0;
> +	bio->bi_iter.bi_size = 0;
> +	if (!pisa_upgrade_user_pages(pages, nr, dir, false)) {
> +		while (nr)
> +			put_page(pages[--nr]);
> +	}
> +	return true;
> +}

[Severity: Medium]
Does filtering out !is_pisa_page(page) prevent O_DIRECT buffers that are
already in a unidirectional PISA allocator from being promoted to
DMA_BIDIRECTIONAL when accessed in the opposite direction?  When an
O_DIRECT buffer allocated for writes (or via MADV_PISA) is later used for
an O_DIRECT read, dma_iova_try_alloc() sees is_pisa_phys(phys) and skips
blk-mq IOVA coalescing, and __pisa_map_phys() then rejects the window
mapping with -EACCES, falling back to individual 4KB IOVA allocations per
segment.

Also, bio_release_pages() and returning true happen before
pisa_upgrade_user_pages() checks whether any page is eligible or within
pisa.user_max_4K_pages budget, forcing bio_iov_iter_get_pages() to unpin
and re-pin the entire bio on every O_DIRECT request whenever migration does
not occur.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index a0c5ddab7a8b6..acedfe04bde26 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1868,7 +1868,7 @@ bool dma_iova_try_alloc(struct device *dev, struct dma_iova_state *state,
>  	dma_addr_t addr;
>  
>  	memset(state, 0, sizeof(*state));
> -	if (!use_dma_iommu(dev) || is_pisa_phys(phys))
> +	if (!use_dma_iommu(dev) || is_pisa_phys(phys))
>  		return false;

[Severity: High]
Can a 4KB PISA user or page-cache page be mapped into an untrusted external
device's IOMMU domain without bounce buffering?

When a 4KB buffer backed by a 2MB PISA frame is used for DMA on an
untrusted PCI/Thunderbolt device (dev_is_untrusted(dev) == true),
dma_iova_try_alloc() returns false because is_pisa_phys(phys) is true, and
iommu_dma_map_phys() skips swiotlb bounce buffering (since the 4KB buffer
is iova_aligned) and calls __pisa_map_phys().  Because neither
dma_iova_try_alloc() nor __pisa_map_phys() rejects untrusted devices,
__pisa_map_phys() installs a persistent 2MB IOMMU leaf PTE in the untrusted
device's domain, exposing the other 511 sibling 4KB user and page-cache
pages in that 2MB frame to DMA by the untrusted device.

> diff --git a/drivers/iommu/pisa-node.c b/drivers/iommu/pisa-node.c
> index cd29c146c281e..7469674133620 100644
> --- a/drivers/iommu/pisa-node.c
> +++ b/drivers/iommu/pisa-node.c
> @@ -184,7 +184,8 @@ void pisa_schedule_scrub(int nid)
>  {
>  	struct pisa_node *node;
>  
> -	if (!READ_ONCE(pisa_scrub_max) || nid == NUMA_NO_NODE)
> +	if ((!READ_ONCE(pisa_scrub_max) && !READ_ONCE(pisa_scrub_migrate)) ||
> +	    nid == NUMA_NO_NODE)
>  		return;

[Severity: Medium]
What schedules pisa_schedule_scrub() when user_pa pages age past
pisa.old_ms and exceed pisa.wasted_pct?

Outside of writing to scrub_max or scrub_migrate in sysfs,
pisa_schedule_scrub() is only called from __pisa_alloc_bulk() and
pisa_alloc_coherent() when pa->scrub_demand >= PISA_SCRUB_TRIGGER.  Because
all user_pa[] allocators enable PCP in pisa_user_init() and
pisa_pcp_alloc() calls __pisa_alloc_bulk() with zero = false,
pa->scrub_demand is never incremented on user_pa[], and neither
__pisa_free_page() nor pisa_reclaim_work_fn() calls pisa_schedule_scrub(),
so background scrub migration never runs automatically.

> diff --git a/drivers/iommu/pisa-user.c b/drivers/iommu/pisa-user.c
> index a16909d429aca..7dca3bf066f0d 100644
> --- a/drivers/iommu/pisa-user.c
> +++ b/drivers/iommu/pisa-user.c
[ ... ]
> +struct folio *__pisa_filemap_alloc(const struct address_space *mapping,
> +				   gfp_t gfp, bool is_write)
> +{
> +	enum dma_data_direction dir = is_write ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +	struct page *page;
> +	int nid;
> +
> +	if (!__pisa_mapping_enabled(mapping, dir))
> +		return NULL;
> +	if (is_write && __pisa_mapping_enabled(mapping, DMA_FROM_DEVICE))
> +		dir = DMA_BIDIRECTIONAL;
> +	/* Pairs with smp_store_release() in pisa_user_init(). */
> +	if (!smp_load_acquire(&pisa_user_ready) &&
> +	    ((gfp & GFP_KERNEL) != GFP_KERNEL || !pisa_user_init()))
> +		return NULL;
> +	if (pisa_user_budget() <= 0)
> +		return NULL;
> +	nid = pisa_sanitize_nid(numa_node_id());
> +	page = pisa_alloc(pisa_nodes[nid].user_pa[dir], gfp);
> +	if (!page)
> +		return NULL;
> +	if (pisa_meta_of_pfn(page_to_pfn(page))->flags & PISA_DECRYPTED) {
> +		put_page(page);
> +		return NULL;
> +	}
> +	return page_folio(page);
> +}

[Severity: Medium]
Can allocating page-cache folios from single-direction user_pa[dir]
allocators cause direction permission mismatches on later I/O?

When only pisa_rx_bufs is enabled on a block device, __pisa_filemap_alloc()
allocates read and readahead folios from user_pa[DMA_FROM_DEVICE]
(IOMMU_WRITE only).  If those folios are later dirtied and written back,
pisa_filemap_upgrade() and pisa_writeback_upgrade() both check
pisa_mapping_enabled(mapping, DMA_TO_DEVICE) and do not promote them;
conversely, when only pisa_tx_bufs is enabled, a partial-page buffered
write allocates a locked folio from user_pa[DMA_TO_DEVICE] (IOMMU_READ
only) and write_begin immediately issues a disk READ into it.  Similarly,
in net/core/datagram.c:pisa_zc_iter(), checking is_pisa_page(pages[i])
without verifying IOMMU_READ allows a DMA_FROM_DEVICE page (such as a
mmap'd page-cache folio, which pisa_upgrade_user_pages() skips because
allow_file is false) to be attached zero-copy instead of copied.  In all of
these cases, dma_iova_try_alloc() sees is_pisa_phys(phys) and disables
blk-mq or TSO IOVA coalescing, only for __pisa_map_phys() to fail with
-EACCES and fall back to per-page 4KB IOVA mappings.

[ ... ]

> +bool __pisa_bdev_enabled(struct block_device *bdev, enum dma_data_direction dir)
> +{
> +	struct device *dev;
> +
> +	if (!bdev || !bdev->bd_disk)
> +		return false;
> +	for (dev = disk_to_dev(bdev->bd_disk); dev; dev = dev->parent) {
> +		if (dir == DMA_FROM_DEVICE ?
> +		    READ_ONCE(dev->pisa_rx_bufs) :
> +		    READ_ONCE(dev->pisa_tx_bufs))
> +			return true;
> +	}
> +	return false;
> +}

[Severity: Medium]
Does gating block device writes on dev->pisa_tx_bufs tie disk write support
to CONFIG_NET and the networking static key?

In drivers/base/core.c, pisa_tx_bufs is wired to net_pisa_tx_bufs_set():

drivers/base/core.c:
    PISA_SET_ATTR(pisa_tx_bufs, net_pisa_tx_bufs_set);

With CONFIG_NET=n, net_pisa_tx_bufs_set() is a stub returning -EOPNOTSUPP,
so pisa_tx_bufs cannot be enabled on any block or storage controller
device.  With CONFIG_NET=y, enabling pisa_tx_bufs on a storage controller
enables net_pisa_tx_bufs_key globally across the networking stack even when
no network device uses PISA.

[Severity: Medium]
Also in __pisa_bdev_enabled(), does walking dev->parent from
disk_to_dev(bdev->bd_disk) work for per-disk configuration or NVMe
multipath devices?

In drivers/base/core.c:device_add_attrs(), dev_attr_pisa_group is only
added when dev->dma_mask is non-NULL, so gendisk devices never expose
pisa_rx_bufs or pisa_tx_bufs in sysfs.  Furthermore, with
CONFIG_NVME_MULTIPATH=y, nvme_mpath_set_live() sets the namespace gendisk's
parent to &head->subsys->dev (nvme-subsysX), which has parent == NULL and
dma_mask == NULL, so walking dev->parent from disk_to_dev(bdev->bd_disk)
never reaches the underlying PCI controller device.

> +
> +bool __pisa_mapping_enabled(const struct address_space *mapping, enum dma_data_direction dir)
> +{
> +	struct super_block *sb;
> +
> +	if (!mapping || !mapping->host || !mapping->host->i_sb)
> +		return false;
> +	sb = mapping->host->i_sb;
> +	return __pisa_bdev_enabled(sb_is_blkdev_sb(sb) ?
> +				   I_BDEV(mapping->host) : sb->s_bdev, dir);
> +}
> +
>  #define PISA_USER_BATCH	512
>  
> +static unsigned long pisa_user_scrub_alloc(struct pisa_alloc *pa,
> +					  enum dma_data_direction dir,
> +					  unsigned int mode,
> +					  unsigned long budget)
> +{
[ ... ]
> +	if (target) {
> +		base_pfn = pisa_meta_to_pfn(target);
> +		bitmap_or(inuse_map, target->clean_bitmap, target->dirty_bitmap, bpp);
> +		bitmap_complement(inuse_map, inuse_map, bpp);
> +		WRITE_ONCE(target->on_list, PISA_LIST_FULL);
> +		list_move(&target->list, &pa->full);
> +	}
> +	spin_unlock_irqrestore(&pa->lock, flags);
> +
> +	if (!target)
> +		return 0;
[ ... ]
> +	if (!list_empty(&list)) {
> +		migrated = pisa_user_migrate_dir(&list, mode == 1, dir, true,
> +						 GFP_KERNEL | __GFP_NOWARN);
> +		pisa_pcp_drain(pa);
> +	}
> +
> +	spin_lock_irqsave(&pa->lock, flags);
> +	if (READ_ONCE(target->pa) == pa) {
> +		WRITE_ONCE(target->alloc_jiffies, jiffies);
> +		should_release = pisa_place_meta(target);
> +	}
> +	spin_unlock_irqrestore(&pa->lock, flags);
> +	if (should_release)
> +		pisa_schedule_reclaim();
> +
> +	return migrated;
> +}

[Severity: High]
Does calling pisa_place_meta(target) after pisa_user_migrate_dir() and
pisa_pcp_drain() corrupt pa->num_idle_pages or double-add target->llnode to
pisa_free_list when all remaining blocks of target are migrated?

When the last in-use block of target is freed during migration or
pisa_pcp_drain(pa), pisa_return_block_locked() already calls
pisa_place_meta(target) with avail == bpp.  In pisa_place_meta(), that
first call either increments pa->num_idle_pages and moves target to
pa->idle, or unlinks target and adds target->llnode to pisa_free_list.
Because target->pa is only cleared in pisa_release_page_rcu() after an RCU
grace period, READ_ONCE(target->pa) == pa is still true here, so
pisa_user_scrub_alloc() calls pisa_place_meta(target) a second time with
avail == bpp:

drivers/iommu/pisa-alloc.c:pisa_place_meta() {
	if (avail == bpp) {
		if ((meta->flags & PISA_QUARANTINED) ||
		    (pa->num_idle_pages >= pa->max_idle_pages && !READ_ONCE(pisa_idle_ms))) {
			list_del_init(&meta->list);
			WRITE_ONCE(meta->on_list, PISA_LIST_NONE);
			llist_add(&meta->llnode, &pisa_free_list);
			return true;
		}
		if (pa->num_idle_pages >= pa->max_idle_pages)
			WRITE_ONCE(pa->idle_jiffies, jiffies);
		pa->num_idle_pages++;
		on_list = meta->nr_dirty ? PISA_LIST_IDLE_DIRTY : PISA_LIST_IDLE_CLEAN;
	}
    ...
	if (on_list == meta->on_list)
		return avail == bpp && pa->num_idle_pages > pa->max_idle_pages;
}

That second call either increments pa->num_idle_pages a second time for the
same page (before the on_list == meta->on_list check), or calls
llist_add(&target->llnode, &pisa_free_list) a second time on the same node
(or moves a page already being released by pisa_reclaim_work_fn() back onto
pa->idle).

In addition, setting target->on_list = PISA_LIST_FULL before migration
makes pisa_page_can_recycle() return true for freed blocks of target, so
__pisa_free_page() caches them in pa->pcp where mode == 1 compaction can
reallocate destination folios right back onto target.

> +
> +bool pisa_user_scrub_node(int nid, unsigned long budget, u64 deadline)
> +{
> +	unsigned int mode = READ_ONCE(pisa_scrub_migrate);
> +	unsigned long done = 0;
> +	unsigned int dir;
> +
> +	/* Pairs with smp_store_release() in pisa_user_init(). */
> +	if (!mode || mode > 2 || !smp_load_acquire(&pisa_user_ready))
> +		return false;
> +
> +	for (dir = 0; dir < ARRAY_SIZE(pisa_nodes[nid].user_pa); dir++) {
> +		struct pisa_alloc *pa = pisa_nodes[nid].user_pa[dir];
> +		unsigned long n;
> +
> +		if (!pa)
> +			continue;
> +		while ((n = pisa_user_scrub_alloc(pa, dir, mode, budget - done))) {
> +			done += n;
> +			if (done >= budget || local_clock() >= deadline)
> +				return true;
> +			cond_resched();
> +		}
> +	}
> +	return false;
> +}

[Severity: Medium]
If the oldest sparse page in pa has only unmigratable folios (for example,
locked, under writeback, pinned, or mapped shared), pisa_user_scrub_alloc()
updates target->alloc_jiffies and returns 0.  Should pisa_user_scrub_node()
continue scanning the remaining old sparse pages in pa instead of exiting
the while loop on the first target page that migrates 0 folios?

Also, if pisa_user_scrub_alloc() stops isolating folios early because count
reaches min(budget - done, PISA_USER_BATCH), resetting
target->alloc_jiffies = jiffies hides the still-sparse page from the
scrubber for pisa.old_ms.

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