Re: [RFC PATCH v2 18/25] iommu/dma: Migrate user pages into PISA for TX zerocopy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] iommu/pisa: unkillable PAGE_SIZE walk under mmap_read_lock in
  pisa_madvise()
- [High] mm/madvise: missing MADV_PISA definitions on alpha, mips,
  parisc, and xtensa
- [Medium] iommu/pisa: alloc_tag corruption via pgalloc_tag_swap() in
  pisa_user_migrate()
- [Medium] iommu/pisa: pisa_user_max_4k_pages not restored on error in
  pisa_user_max_set()
- [Medium] net: pisa_zc_iter() migrates user pages before checking
  dst_tx_pisa_ok()
--

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

iommu/dma: Migrate user pages into PISA for TX zerocopy

This commit adds support for migrating private anonymous order-0 user pages
into per-node PISA allocators via madvise(MADV_PISA / MADV_NOPISA) and
automatically during MSG_ZEROCOPY transmissions.

> diff --git a/drivers/iommu/pisa-user.c b/drivers/iommu/pisa-user.c
> new file mode 100644
> index 0000000000000..a16909d429aca
> --- /dev/null
> +++ b/drivers/iommu/pisa-user.c
> @@ -0,0 +1,278 @@

[ ... ]

> +static int pisa_user_max_set(const char *val, const struct kernel_param *kp)
> +{
> +	unsigned long max_pages;
> +	int ret;
> +
> +	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
> +		return -EOPNOTSUPP;
> +	ret = kstrtoul(val, 0, &max_pages);
> +	if (ret)
> +		return ret;
> +	WRITE_ONCE(pisa_user_max_4k_pages, max_pages);
> +	if (max_pages) {
> +		if (!pisa_nodes[first_memory_node].pa[0][0])
> +			return 0;
> +		if (!pisa_user_init())
> +			return -ENOMEM;
> +		static_branch_enable(&pisa_user_key);
> +	} else {
> +		static_branch_disable(&pisa_user_key);
> +	}
> +	return 0;
> +}

[Severity: Medium]
Does this leave pisa_user_max_4k_pages updated if pisa_user_init() fails and
returns -ENOMEM?  Sysfs will report the new non-zero limit after the write
failed, and a later madvise(MADV_PISA) call can then succeed in
pisa_user_init() while pisa_user_key remains disabled.

[ ... ]

> +/* new_folio_t for migration into PISA memory. */
> +static struct folio *pisa_user_new_folio(struct folio *src, unsigned long private)
> +{
> +	struct pisa_user_mtc *mtc = (void *)private;
> +	int nid = folio_nid(src);
> +	struct page *page;
> +
> +	if (mtc->budget <= 0)
> +		return NULL;
> +	page = pisa_alloc(pisa_nodes[nid].user_pa, mtc->gfp);
> +	if (!page)
> +		return NULL;
> +	if (pisa_meta_of_pfn(page_to_pfn(page))->flags & PISA_DECRYPTED) {
> +		put_page(page);
> +		mtc->budget = 0;
> +		return NULL;
> +	}
> +	mtc->budget--;
> +	return page_folio(page);
> +}
> +
> +/* new_folio_t for migration out of PISA memory: allocators never serve these. */
> +static struct folio *pisa_user_old_folio(struct folio *src, unsigned long private)
> +{
> +	return __folio_alloc_node(GFP_HIGHUSER_MOVABLE | __GFP_NOWARN, 0, folio_nid(src));
> +}
> +
> +/* Migrate the isolated folios on @list; failures are put back. */
> +static void pisa_user_migrate(struct list_head *list, bool to_pisa, gfp_t gfp)
> +{
> +	struct pisa_user_mtc mtc = { .gfp = gfp, .budget = LONG_MAX };
> +	enum migrate_mode mode = MIGRATE_SYNC;
> +
> +	if (list_empty(list))
> +		return;
> +	if (to_pisa) {
> +		mtc.budget = pisa_user_budget();
> +		if (!gfpflags_allow_blocking(gfp))
> +			mode = MIGRATE_ASYNC;
> +	}
> +	if (migrate_pages(list, to_pisa ? pisa_user_new_folio : pisa_user_old_folio,
> +			  NULL, (unsigned long)&mtc, mode, MR_SYSCALL, NULL))
> +		putback_movable_pages(list);
> +}

[Severity: Medium]
Does migrate_pages() corrupt page allocation profiling counters when
CONFIG_MEM_ALLOC_PROFILING is enabled?

When a 2MB PISA page is allocated in __pisa_page_alloc() and split in
split_page_compound(), pgalloc_tag_split() assigns __pisa_page_alloc()'s
alloc_tag to all 512 blocks, and __pisa_free_page() recycles individual
blocks without calling pgalloc_tag_sub().  However, migrate_pages() calls
folio_migrate_flags() -> pgalloc_tag_swap(dst, src).  Migrating a user folio
into a PISA block swaps __pisa_page_alloc()'s tag onto the old buddy folio
(which is immediately freed, decrementing __pisa_page_alloc()'s tag while
the PISA block is still allocated) and places the user folio's tag onto the
PISA block, where it is never decremented when __pisa_free_page() recycles
the block.  Migrating out via MADV_NOPISA similarly leaves
pisa_user_old_folio()'s tag stranded on the recycled PISA block.

[ ... ]

> +int pisa_madvise(struct vm_area_struct *vma, unsigned long start, unsigned long end, bool to_pisa)
> +{
> +	gfp_t gfp = GFP_USER | __GFP_NOWARN;
> +	struct folio_walk fw;
> +	unsigned long addr;
> +	unsigned int n = 0;
> +	LIST_HEAD(list);
> +
> +	if (vma->vm_flags & (VM_SHARED | VM_IO | VM_PFNMAP | VM_HUGETLB))
> +		return -EINVAL;
> +	if (to_pisa && !pisa_user_init())
> +		return -EOPNOTSUPP;
> +	lru_add_drain();
> +	for (addr = start; addr < end; addr += PAGE_SIZE) {
> +		struct folio *folio = folio_walk_start(&fw, vma, addr, 0);
> +
> +		if (folio) {
> +			if (pisa_user_want(folio, to_pisa) && !pisa_user_busy(folio, 0) &&
> +			    isolate_folio_to_list(folio, &list))
> +				n++;
> +			folio_walk_end(&fw, vma);
> +		}
> +		if (n == PISA_USER_BATCH) {
> +			pisa_user_migrate(&list, to_pisa, gfp);
> +			n = 0;
> +		}
> +		cond_resched();
> +	}
> +	pisa_user_migrate(&list, to_pisa, gfp);
> +	return 0;
> +}

[Severity: High]
Can an unprivileged user stall a task in an unkillable loop holding
mmap_read_lock by calling madvise(MADV_NOPISA) or madvise(MADV_PISA) on a
multi-terabyte unpopulated anonymous mapping?

Unlike walk_page_range_vma(), which skips empty PGD, P4D, PUD, and PMD
entries at once, this loop steps by PAGE_SIZE across the entire [start, end)
range and calls folio_walk_start() on every 4KB address without checking
fatal_signal_pending(), and MADV_NOPISA reaches it even when
pisa.user_max_4K_pages is 0.  Using walk_page_range_vma() or checking
pisa_user_enabled() and fatal_signal_pending() would avoid walking empty
page table levels and allow SIGKILL to abort the walk.

> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index ef1c27fa3c570..c4332db8e56ff 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -81,6 +81,8 @@
>  
>  #define MADV_GUARD_INSTALL 102		/* fatal signal on access to range */
>  #define MADV_GUARD_REMOVE 103		/* unguard range */
> +#define MADV_PISA		210	/* move pages into PISA memory */
> +#define MADV_NOPISA		211	/* move pages out of PISA memory */

[Severity: High]
Does mm/madvise.c fail to build on alpha, mips, parisc, and xtensa?

Those four architectures do not include asm-generic/mman-common.h in their
arch/*/include/uapi/asm/mman.h headers, while mm/madvise.c references
MADV_PISA and MADV_NOPISA unconditionally.

> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index ecc30a6f83023..40f20d0faa859 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c

[ ... ]

> +static bool pisa_zc_iter(int *ret, struct sk_buff *skb, struct iov_iter *from, size_t *length,
> +			 const struct sock *sk, struct page **pages, int max_pages,
> +			 unsigned int orig_len, size_t *start, ssize_t *copied)
> +{
> +	unsigned int i, nr;
> +	ssize_t keep;
> +
> +	if (!pisa_user_enabled() || !user_backed_iter(from))
> +		return false;
> +
> +	/* Move user pages into PISA memory, then look them up again. */
> +	if (pisa_upgrade_user_pages(pages, DIV_ROUND_UP(*start + *copied, PAGE_SIZE))) {
> +		iov_iter_revert(from, *copied);
> +		*copied = iov_iter_get_pages2(from, pages, *length, max_pages, start);
> +		if (*copied < 0) {
> +			*ret = -EFAULT;
> +			return true;
> +		}
> +	}
> +
> +	/* Only TCP (which sets skb->sk at transmit time) handles short appends/-EXDEV. */
> +	if (!sk || skb->sk)
> +		return false;
> +
> +	/* Only plain MSG_ZEROCOPY ubufs (not io_uring/vhost/devmem). */
> +	if (skb_zcopy(skb) && skb_zcopy(skb)->ops != &msg_zerocopy_ubuf_ops)
> +		return false;
> +
> +	/* Egress device must have pisa_tx_bufs enabled so the copy uses a PISA page_frag. */
> +	if (!dst_tx_pisa_ok(__sk_dst_get(sk)))
> +		return false;

[Severity: Medium]
Should pisa_zc_iter() check sk and dst_tx_pisa_ok(__sk_dst_get(sk)) before
calling pisa_upgrade_user_pages()?

Calling pisa_upgrade_user_pages() first migrates user pages into PISA even
when the egress device has pisa_tx_bufs disabled, when sending over
loopback or virtual devices, or when the socket is routed over IPsec
(where __dst_tx_pisa_ok() excludes dst_xfrm(dst) to keep plaintext out of
shared 2MB PISA pages).  Because __iommu_dma_map() checks is_pisa_phys()
rather than dev->pisa_tx_bufs, migrating pages on a device with
pisa_tx_bufs disabled also causes pisa_map_phys() to install 2MB PISA IOMMU
mappings on that device.

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