[net-next,v2,3/3] page_pool: Track DMA-mapped pages and unmap them when destroying the pool

Commit Message

Toke Høiland-Jørgensen March 25, 2025, 3:45 p.m. UTC

When enabling DMA mapping in page_pool, pages are kept DMA mapped until
they are released from the pool, to avoid the overhead of re-mapping the
pages every time they are used. This causes resource leaks and/or
crashes when there are pages still outstanding while the device is torn
down, because page_pool will attempt an unmap through a non-existent DMA
device on the subsequent page return.

To fix this, implement a simple tracking of outstanding DMA-mapped pages
in page pool using an xarray. This was first suggested by Mina[0], and
turns out to be fairly straight forward: We simply store pointers to
pages directly in the xarray with xa_alloc() when they are first DMA
mapped, and remove them from the array on unmap. Then, when a page pool
is torn down, it can simply walk the xarray and unmap all pages still
present there before returning, which also allows us to get rid of the
get/put_device() calls in page_pool. Using xa_cmpxchg(), no additional
synchronisation is needed, as a page will only ever be unmapped once.

To avoid having to walk the entire xarray on unmap to find the page
reference, we stash the ID assigned by xa_alloc() into the page
structure itself, using the upper bits of the pp_magic field. This
requires a couple of defines to avoid conflicting with the
POINTER_POISON_DELTA define, but this is all evaluated at compile-time,
so does not affect run-time performance. The bitmap calculations in this
patch gives the following number of bits for different architectures:

- 23 bits on 32-bit architectures
- 21 bits on PPC64 (because of the definition of ILLEGAL_POINTER_VALUE)
- 32 bits on other 64-bit architectures

Stashing a value into the unused bits of pp_magic does have the effect
that it can make the value stored there lie outside the unmappable
range (as governed by the mmap_min_addr sysctl), for architectures that
don't define ILLEGAL_POINTER_VALUE. This means that if one of the
pointers that is aliased to the pp_magic field (such as page->lru.next)
is dereferenced while the page is owned by page_pool, that could lead to
a dereference into userspace, which is a security concern. The risk of
this is mitigated by the fact that (a) we always clear pp_magic before
releasing a page from page_pool, and (b) this would need a
use-after-free bug for struct page, which can have many other risks
since page->lru.next is used as a generic list pointer in multiple
places in the kernel. As such, with this patch we take the position that
this risk is negligible in practice. For more discussion, see[1].

Since all the tracking added in this patch is performed on DMA
map/unmap, no additional code is needed in the fast path, meaning the
performance overhead of this tracking is negligible there. A
micro-benchmark shows that the total overhead of the tracking itself is
about 400 ns (39 cycles(tsc) 395.218 ns; sum for both map and unmap[2]).
Since this cost is only paid on DMA map and unmap, it seems like an
acceptable cost to fix the late unmap issue. Further optimisation can
narrow the cases where this cost is paid (for instance by eliding the
tracking when DMA map/unmap is a no-op).

The extra memory needed to track the pages is neatly encapsulated inside
xarray, which uses the 'struct xa_node' structure to track items. This
structure is 576 bytes long, with slots for 64 items, meaning that a
full node occurs only 9 bytes of overhead per slot it tracks (in
practice, it probably won't be this efficient, but in any case it should
be an acceptable overhead).

[0] https://lore.kernel.org/all/[e-mail]/
[1] https://lore.kernel.org/r/[e-mail]
[2] https://lore.kernel.org/r/[e-mail]

Reported-by: Yonglong Liu <[e-mail]>
Closes: https://lore.kernel.org/r/[e-mail]
Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
Suggested-by: Mina Almasry <[e-mail]>
Reviewed-by: Mina Almasry <[e-mail]>
Reviewed-by: Jesper Dangaard Brouer <[e-mail]>
Tested-by: Jesper Dangaard Brouer <[e-mail]>
Tested-by: Qiuling Ren <[e-mail]>
Tested-by: Yuying Ma <[e-mail]>
Tested-by: Yonglong Liu <[e-mail]>
Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
---
 include/linux/poison.h        |  4 +++
 include/net/page_pool/types.h | 49 +++++++++++++++++++++++---
 net/core/netmem_priv.h        | 28 ++++++++++++++-
 net/core/page_pool.c          | 82 ++++++++++++++++++++++++++++++++++++-------
 4 files changed, 145 insertions(+), 18 deletions(-)

Comments

Jesper Dangaard Brouer March 26, 2025, 1:54 p.m. UTC | #1

On 25/03/2025 16.45, Toke Høiland-Jørgensen wrote:
> When enabling DMA mapping in page_pool, pages are kept DMA mapped until
> they are released from the pool, to avoid the overhead of re-mapping the
> pages every time they are used. This causes resource leaks and/or
> crashes when there are pages still outstanding while the device is torn
> down, because page_pool will attempt an unmap through a non-existent DMA
> device on the subsequent page return.
> 
> To fix this, implement a simple tracking of outstanding DMA-mapped pages
> in page pool using an xarray. This was first suggested by Mina[0], and
> turns out to be fairly straight forward: We simply store pointers to
> pages directly in the xarray with xa_alloc() when they are first DMA
> mapped, and remove them from the array on unmap. Then, when a page pool
> is torn down, it can simply walk the xarray and unmap all pages still
> present there before returning, which also allows us to get rid of the
> get/put_device() calls in page_pool. Using xa_cmpxchg(), no additional
> synchronisation is needed, as a page will only ever be unmapped once.
> 
> To avoid having to walk the entire xarray on unmap to find the page
> reference, we stash the ID assigned by xa_alloc() into the page
> structure itself, using the upper bits of the pp_magic field. This
> requires a couple of defines to avoid conflicting with the
> POINTER_POISON_DELTA define, but this is all evaluated at compile-time,
> so does not affect run-time performance. The bitmap calculations in this
> patch gives the following number of bits for different architectures:
> 
> - 23 bits on 32-bit architectures
> - 21 bits on PPC64 (because of the definition of ILLEGAL_POINTER_VALUE)
> - 32 bits on other 64-bit architectures
> 
> Stashing a value into the unused bits of pp_magic does have the effect
> that it can make the value stored there lie outside the unmappable
> range (as governed by the mmap_min_addr sysctl), for architectures that
> don't define ILLEGAL_POINTER_VALUE. This means that if one of the
> pointers that is aliased to the pp_magic field (such as page->lru.next)
> is dereferenced while the page is owned by page_pool, that could lead to
> a dereference into userspace, which is a security concern. The risk of
> this is mitigated by the fact that (a) we always clear pp_magic before
> releasing a page from page_pool, and (b) this would need a
> use-after-free bug for struct page, which can have many other risks
> since page->lru.next is used as a generic list pointer in multiple
> places in the kernel. As such, with this patch we take the position that
> this risk is negligible in practice. For more discussion, see[1].
> 
> Since all the tracking added in this patch is performed on DMA
> map/unmap, no additional code is needed in the fast path, meaning the
> performance overhead of this tracking is negligible there. A
> micro-benchmark shows that the total overhead of the tracking itself is
> about 400 ns (39 cycles(tsc) 395.218 ns; sum for both map and unmap[2]).
> Since this cost is only paid on DMA map and unmap, it seems like an
> acceptable cost to fix the late unmap issue. Further optimisation can
> narrow the cases where this cost is paid (for instance by eliding the
> tracking when DMA map/unmap is a no-op).
> 
> The extra memory needed to track the pages is neatly encapsulated inside
> xarray, which uses the 'struct xa_node' structure to track items. This
> structure is 576 bytes long, with slots for 64 items, meaning that a
> full node occurs only 9 bytes of overhead per slot it tracks (in
> practice, it probably won't be this efficient, but in any case it should
> be an acceptable overhead).
> 
> [0]https://lore.kernel.org/all/[e-mail]/
> [1]https://lore.kernel.org/r/[e-mail]
> [2]https://lore.kernel.org/r/[e-mail]
> 
> Reported-by: Yonglong Liu<[e-mail]>
> Closes:https://lore.kernel.org/r/[e-mail]
> Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
> Suggested-by: Mina Almasry<[e-mail]>
> Reviewed-by: Mina Almasry<[e-mail]>
> Reviewed-by: Jesper Dangaard Brouer<[e-mail]>
> Tested-by: Jesper Dangaard Brouer<[e-mail]>
> Tested-by: Qiuling Ren<[e-mail]>
> Tested-by: Yuying Ma<[e-mail]>
> Tested-by: Yonglong Liu<[e-mail]>
> Signed-off-by: Toke Høiland-Jørgensen<[e-mail]>
> ---
>   include/linux/poison.h        |  4 +++
>   include/net/page_pool/types.h | 49 +++++++++++++++++++++++---
>   net/core/netmem_priv.h        | 28 ++++++++++++++-
>   net/core/page_pool.c          | 82 ++++++++++++++++++++++++++++++++++++-------
>   4 files changed, 145 insertions(+), 18 deletions(-)


Acked-by: Jesper Dangaard Brouer <[e-mail]>

Saeed Mahameed March 26, 2025, 6:22 p.m. UTC | #2

On 25 Mar 16:45, Toke Høiland-Jørgensen wrote:
>When enabling DMA mapping in page_pool, pages are kept DMA mapped until
>they are released from the pool, to avoid the overhead of re-mapping the
>pages every time they are used. This causes resource leaks and/or
>crashes when there are pages still outstanding while the device is torn
>down, because page_pool will attempt an unmap through a non-existent DMA
>device on the subsequent page return.
>

Why dynamically track when it is guaranteed the page_pool consumer (driver) 
will return all outstanding pages before disabling the DMA device.
When a page pool is destroyed by the driver, just mark it as "DMA-inactive",
and on page_pool_return_page() if DMA-inactive don't recycle those pages 
and immediately DMA unmap and release them. We can also track drivers if
they still have page_pools with outstanding DMA, and print a warning when
DMA is disabled.

I know there's an extra check on fast path, but looking below it seems
like slow path is becoming unnecessarily complicated, and we are sacrificing
slow path performance significantly for the sake of not touching fast path
at all. page_pool slow path should _not_ be significantly slower
than using the page allocator directly! In many production environments and
some workloads, page pool recycling can happen at a very low rate resulting
on performance relying solely on slow path.. 

>To fix this, implement a simple tracking of outstanding DMA-mapped pages
>in page pool using an xarray. This was first suggested by Mina[0], and
>turns out to be fairly straight forward: We simply store pointers to
>pages directly in the xarray with xa_alloc() when they are first DMA
>mapped, and remove them from the array on unmap. Then, when a page pool
>is torn down, it can simply walk the xarray and unmap all pages still
>present there before returning, which also allows us to get rid of the
>get/put_device() calls in page_pool. Using xa_cmpxchg(), no additional
>synchronisation is needed, as a page will only ever be unmapped once.
>
>To avoid having to walk the entire xarray on unmap to find the page
>reference, we stash the ID assigned by xa_alloc() into the page
>structure itself, using the upper bits of the pp_magic field. This
>requires a couple of defines to avoid conflicting with the
>POINTER_POISON_DELTA define, but this is all evaluated at compile-time,
>so does not affect run-time performance. The bitmap calculations in this
>patch gives the following number of bits for different architectures:
>
>- 23 bits on 32-bit architectures
>- 21 bits on PPC64 (because of the definition of ILLEGAL_POINTER_VALUE)
>- 32 bits on other 64-bit architectures
>
>Stashing a value into the unused bits of pp_magic does have the effect
>that it can make the value stored there lie outside the unmappable
>range (as governed by the mmap_min_addr sysctl), for architectures that
>don't define ILLEGAL_POINTER_VALUE. This means that if one of the
>pointers that is aliased to the pp_magic field (such as page->lru.next)
>is dereferenced while the page is owned by page_pool, that could lead to
>a dereference into userspace, which is a security concern. The risk of
>this is mitigated by the fact that (a) we always clear pp_magic before
>releasing a page from page_pool, and (b) this would need a
>use-after-free bug for struct page, which can have many other risks
>since page->lru.next is used as a generic list pointer in multiple
>places in the kernel. As such, with this patch we take the position that
>this risk is negligible in practice. For more discussion, see[1].
>
>Since all the tracking added in this patch is performed on DMA
>map/unmap, no additional code is needed in the fast path, meaning the
>performance overhead of this tracking is negligible there. A
>micro-benchmark shows that the total overhead of the tracking itself is
>about 400 ns (39 cycles(tsc) 395.218 ns; sum for both map and unmap[2]).
>Since this cost is only paid on DMA map and unmap, it seems like an
>acceptable cost to fix the late unmap issue. Further optimisation can
>narrow the cases where this cost is paid (for instance by eliding the
>tracking when DMA map/unmap is a no-op).
>
What I am missing here, what is the added cost of those extra operations on
the slow path compared to before this patch? Total overhead being
acceptable doesn't justify the change, we need diff before and after.

>The extra memory needed to track the pages is neatly encapsulated inside
>xarray, which uses the 'struct xa_node' structure to track items. This
>structure is 576 bytes long, with slots for 64 items, meaning that a
>full node occurs only 9 bytes of overhead per slot it tracks (in
>practice, it probably won't be this efficient, but in any case it should
>be an acceptable overhead).
>
>[0] https://lore.kernel.org/all/[e-mail]/
>[1] https://lore.kernel.org/r/[e-mail]
>[2] https://lore.kernel.org/r/[e-mail]
>
>Reported-by: Yonglong Liu <[e-mail]>
>Closes: https://lore.kernel.org/r/[e-mail]
>Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
>Suggested-by: Mina Almasry <[e-mail]>
>Reviewed-by: Mina Almasry <[e-mail]>
>Reviewed-by: Jesper Dangaard Brouer <[e-mail]>
>Tested-by: Jesper Dangaard Brouer <[e-mail]>
>Tested-by: Qiuling Ren <[e-mail]>
>Tested-by: Yuying Ma <[e-mail]>
>Tested-by: Yonglong Liu <[e-mail]>
>Signed-off-by: Toke Høiland-Jørgensen <[e-mail]>
>---
> include/linux/poison.h        |  4 +++
> include/net/page_pool/types.h | 49 +++++++++++++++++++++++---
> net/core/netmem_priv.h        | 28 ++++++++++++++-
> net/core/page_pool.c          | 82 ++++++++++++++++++++++++++++++++++++-------
> 4 files changed, 145 insertions(+), 18 deletions(-)
>
>diff --git a/include/linux/poison.h b/include/linux/poison.h
>index 331a9a996fa8746626afa63ea462b85ca3e5938b..5351efd710d5e21cc341f7bb533b1aeea4a0808a 100644
>--- a/include/linux/poison.h
>+++ b/include/linux/poison.h
>@@ -70,6 +70,10 @@
> #define KEY_DESTROY		0xbd
>
> /********** net/core/page_pool.c **********/
>+/*
>+ * page_pool uses additional free bits within this value to store data, see the
>+ * definition of PP_DMA_INDEX_MASK in include/net/page_pool/types.h
>+ */
> #define PP_SIGNATURE		(0x40 + POISON_POINTER_DELTA)
>
> /********** net/core/skbuff.c **********/
>diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
>index fbe34024b20061e8bcd1d4474f6ebfc70992f1eb..8f9ed92a4b2af6c136406c41b1a829c8e52ba3e2 100644
>--- a/include/net/page_pool/types.h
>+++ b/include/net/page_pool/types.h
>@@ -6,6 +6,7 @@
> #include <linux/dma-direction.h>
> #include <linux/ptr_ring.h>
> #include <linux/types.h>
>+#include <linux/xarray.h>
> #include <net/netmem.h>
>
> #define PP_FLAG_DMA_MAP		BIT(0) /* Should page_pool do the DMA
>@@ -58,13 +59,51 @@ struct pp_alloc_cache {
> 	netmem_ref cache[PP_ALLOC_CACHE_SIZE];
> };
>
>+/*
>+ * DMA mapping IDs
>+ *
>+ * When DMA-mapping a page, we allocate an ID (from an xarray) and stash this in
>+ * the upper bits of page->pp_magic. We always want to be able to unambiguously
>+ * identify page pool pages (using page_pool_page_is_pp()). Non-PP pages can
>+ * have arbitrary kernel pointers stored in the same field as pp_magic (since it
>+ * overlaps with page->lru.next), so we must ensure that we cannot mistake a
>+ * valid kernel pointer with any of the values we write into this field.
>+ *
>+ * On architectures that set POISON_POINTER_DELTA, this is already ensured,
>+ * since this value becomes part of PP_SIGNATURE; meaning we can just use the
>+ * space between the PP_SIGNATURE value (without POISON_POINTER_DELTA), and the
>+ * lowest bits of POISON_POINTER_DELTA. On arches where POISON_POINTER_DELTA is
>+ * 0, we make sure that we leave the two topmost bits empty, as that guarantees
>+ * we won't mistake a valid kernel pointer for a value we set, regardless of the
>+ * VMSPLIT setting.
>+ *
>+ * Altogether, this means that the number of bits available is constrained by
>+ * the size of an unsigned long (at the upper end, subtracting two bits per the
>+ * above), and the definition of PP_SIGNATURE (with or without
>+ * POISON_POINTER_DELTA).
>+ */
>+#define PP_DMA_INDEX_SHIFT (1 + __fls(PP_SIGNATURE - POISON_POINTER_DELTA))
>+#if POISON_POINTER_DELTA > 0
>+/* PP_SIGNATURE includes POISON_POINTER_DELTA, so limit the size of the DMA
>+ * index to not overlap with that if set
>+ */
>+#define PP_DMA_INDEX_BITS MIN(32, __ffs(POISON_POINTER_DELTA) - PP_DMA_INDEX_SHIFT)
>+#else
>+/* Always leave out the topmost two; see above. */
>+#define PP_DMA_INDEX_BITS MIN(32, BITS_PER_LONG - PP_DMA_INDEX_SHIFT - 2)
>+#endif
>+
>+#define PP_DMA_INDEX_MASK GENMASK(PP_DMA_INDEX_BITS + PP_DMA_INDEX_SHIFT - 1, \
>+				  PP_DMA_INDEX_SHIFT)
>+#define PP_DMA_INDEX_LIMIT XA_LIMIT(1, BIT(PP_DMA_INDEX_BITS) - 1)
>+
> /* Mask used for checking in page_pool_page_is_pp() below. page->pp_magic is
>  * OR'ed with PP_SIGNATURE after the allocation in order to preserve bit 0 for
>- * the head page of compound page and bit 1 for pfmemalloc page.
>- * page_is_pfmemalloc() is checked in __page_pool_put_page() to avoid recycling
>- * the pfmemalloc page.
>+ * the head page of compound page and bit 1 for pfmemalloc page, as well as the
>+ * bits used for the DMA index. page_is_pfmemalloc() is checked in
>+ * __page_pool_put_page() to avoid recycling the pfmemalloc page.
>  */
>-#define PP_MAGIC_MASK ~0x3UL
>+#define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>
> /**
>  * struct page_pool_params - page pool parameters
>@@ -233,6 +272,8 @@ struct page_pool {
> 	void *mp_priv;
> 	const struct memory_provider_ops *mp_ops;
>
>+	struct xarray dma_mapped;
>+
> #ifdef CONFIG_PAGE_POOL_STATS
> 	/* recycle stats are per-cpu to avoid locking */
> 	struct page_pool_recycle_stats __percpu *recycle_stats;
>diff --git a/net/core/netmem_priv.h b/net/core/netmem_priv.h
>index f33162fd281c23e109273ba09950c5d0a2829bc9..cd95394399b40c3604934ba7898eeeeacb8aee99 100644
>--- a/net/core/netmem_priv.h
>+++ b/net/core/netmem_priv.h
>@@ -5,7 +5,7 @@
>
> static inline unsigned long netmem_get_pp_magic(netmem_ref netmem)
> {
>-	return __netmem_clear_lsb(netmem)->pp_magic;
>+	return __netmem_clear_lsb(netmem)->pp_magic & ~PP_DMA_INDEX_MASK;
> }
>
> static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic)
>@@ -15,6 +15,8 @@ static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic)
>
> static inline void netmem_clear_pp_magic(netmem_ref netmem)
> {
>+	WARN_ON_ONCE(__netmem_clear_lsb(netmem)->pp_magic & PP_DMA_INDEX_MASK);
>+
> 	__netmem_clear_lsb(netmem)->pp_magic = 0;
> }
>
>@@ -33,4 +35,28 @@ static inline void netmem_set_dma_addr(netmem_ref netmem,
> {
> 	__netmem_clear_lsb(netmem)->dma_addr = dma_addr;
> }
>+
>+static inline unsigned long netmem_get_dma_index(netmem_ref netmem)
>+{
>+	unsigned long magic;
>+
>+	if (WARN_ON_ONCE(netmem_is_net_iov(netmem)))
>+		return 0;
>+
>+	magic = __netmem_clear_lsb(netmem)->pp_magic;
>+
>+	return (magic & PP_DMA_INDEX_MASK) >> PP_DMA_INDEX_SHIFT;
>+}
>+
>+static inline void netmem_set_dma_index(netmem_ref netmem,
>+					unsigned long id)
>+{
>+	unsigned long magic;
>+
>+	if (WARN_ON_ONCE(netmem_is_net_iov(netmem)))
>+		return;
>+
>+	magic = netmem_get_pp_magic(netmem) | (id << PP_DMA_INDEX_SHIFT);
>+	__netmem_clear_lsb(netmem)->pp_magic = magic;
>+}
> #endif
>diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>index d51ca4389dd62d8bc266a9a2b792838257173535..55acb4bc2c57893486f222e9f39b48a09b0d78d0 100644
>--- a/net/core/page_pool.c
>+++ b/net/core/page_pool.c
>@@ -226,6 +226,8 @@ static int page_pool_init(struct page_pool *pool,
> 			return -EINVAL;
>
> 		pool->dma_map = true;
>+
>+		xa_init_flags(&pool->dma_mapped, XA_FLAGS_ALLOC1);
> 	}
>
> 	if (pool->slow.flags & PP_FLAG_DMA_SYNC_DEV) {
>@@ -275,9 +277,6 @@ static int page_pool_init(struct page_pool *pool,
> 	/* Driver calling page_pool_create() also call page_pool_destroy() */
> 	refcount_set(&pool->user_cnt, 1);
>
>-	if (pool->dma_map)
>-		get_device(pool->p.dev);
>-
> 	if (pool->slow.flags & PP_FLAG_ALLOW_UNREADABLE_NETMEM) {
> 		/* We rely on rtnl_lock()ing to make sure netdev_rx_queue
> 		 * configuration doesn't change while we're initializing
>@@ -325,7 +324,7 @@ static void page_pool_uninit(struct page_pool *pool)
> 	ptr_ring_cleanup(&pool->ring, NULL);
>
> 	if (pool->dma_map)
>-		put_device(pool->p.dev);
>+		xa_destroy(&pool->dma_mapped);
>
> #ifdef CONFIG_PAGE_POOL_STATS
> 	if (!pool->system)
>@@ -466,14 +465,20 @@ page_pool_dma_sync_for_device(const struct page_pool *pool,
> 			      netmem_ref netmem,
> 			      u32 dma_sync_size)
> {
>-	if ((READ_ONCE(pool->dma_sync) & PP_DMA_SYNC_DEV) &&
>-	    dma_dev_need_sync(pool->p.dev))
>-		__page_pool_dma_sync_for_device(pool, netmem, dma_sync_size);
>+	if (dma_dev_need_sync(pool->p.dev)) {
>+		rcu_read_lock();
>+		if (READ_ONCE(pool->dma_sync) & PP_DMA_SYNC_DEV)
>+			__page_pool_dma_sync_for_device(pool, netmem,
>+							dma_sync_size);
>+		rcu_read_unlock();
>+	}
> }
>
>-static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
>+static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t gfp)
> {
> 	dma_addr_t dma;
>+	int err;
>+	u32 id;
>
> 	/* Setup DMA mapping: use 'struct page' area for storing DMA-addr
> 	 * since dma_addr_t can be either 32 or 64 bits and does not always fit
>@@ -487,15 +492,28 @@ static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
> 	if (dma_mapping_error(pool->p.dev, dma))
> 		return false;
>
>-	if (page_pool_set_dma_addr_netmem(netmem, dma))
>+	if (in_softirq())
>+		err = xa_alloc(&pool->dma_mapped, &id, netmem_to_page(netmem),
>+			       PP_DMA_INDEX_LIMIT, gfp);
>+	else
>+		err = xa_alloc_bh(&pool->dma_mapped, &id, netmem_to_page(netmem),
>+				  PP_DMA_INDEX_LIMIT, gfp);
>+	if (err) {
>+		WARN_ONCE(1, "couldn't track DMA mapping, please report to netdev@");
> 		goto unmap_failed;
>+	}
>
>+	if (page_pool_set_dma_addr_netmem(netmem, dma)) {
>+		WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
>+		goto unmap_failed;
>+	}
>+
>+	netmem_set_dma_index(netmem, id);
> 	page_pool_dma_sync_for_device(pool, netmem, pool->p.max_len);
>
> 	return true;
>
> unmap_failed:
>-	WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
> 	dma_unmap_page_attrs(pool->p.dev, dma,
> 			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> 			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
>@@ -512,7 +530,7 @@ static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
> 	if (unlikely(!page))
> 		return NULL;
>
>-	if (pool->dma_map && unlikely(!page_pool_dma_map(pool, page_to_netmem(page)))) {
>+	if (pool->dma_map && unlikely(!page_pool_dma_map(pool, page_to_netmem(page), gfp))) {
> 		put_page(page);
> 		return NULL;
> 	}
>@@ -558,7 +576,7 @@ static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
> 	 */
> 	for (i = 0; i < nr_pages; i++) {
> 		netmem = pool->alloc.cache[i];
>-		if (dma_map && unlikely(!page_pool_dma_map(pool, netmem))) {
>+		if (dma_map && unlikely(!page_pool_dma_map(pool, netmem, gfp))) {
> 			put_page(netmem_to_page(netmem));
> 			continue;
> 		}
>@@ -660,6 +678,8 @@ void page_pool_clear_pp_info(netmem_ref netmem)
> static __always_inline void __page_pool_release_page_dma(struct page_pool *pool,
> 							 netmem_ref netmem)
> {
>+	struct page *old, *page = netmem_to_page(netmem);
>+	unsigned long id;
> 	dma_addr_t dma;
>
> 	if (!pool->dma_map)
>@@ -668,6 +688,17 @@ static __always_inline void __page_pool_release_page_dma(struct page_pool *pool,
> 		 */
> 		return;
>
>+	id = netmem_get_dma_index(netmem);
>+	if (!id)
>+		return;
>+
>+	if (in_softirq())
>+		old = xa_cmpxchg(&pool->dma_mapped, id, page, NULL, 0);
>+	else
>+		old = xa_cmpxchg_bh(&pool->dma_mapped, id, page, NULL, 0);
>+	if (old != page)
>+		return;
>+
> 	dma = page_pool_get_dma_addr_netmem(netmem);
>
> 	/* When page is unmapped, it cannot be returned to our pool */
>@@ -675,6 +706,7 @@ static __always_inline void __page_pool_release_page_dma(struct page_pool *pool,
> 			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> 			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
> 	page_pool_set_dma_addr_netmem(netmem, 0);
>+	netmem_set_dma_index(netmem, 0);
> }
>
> /* Disconnects a page (from a page_pool).  API users can have a need
>@@ -1084,8 +1116,32 @@ static void page_pool_empty_alloc_cache_once(struct page_pool *pool)
>
> static void page_pool_scrub(struct page_pool *pool)
> {
>+	unsigned long id;
>+	void *ptr;
>+
> 	page_pool_empty_alloc_cache_once(pool);
>-	pool->destroy_cnt++;
>+	if (!pool->destroy_cnt++ && pool->dma_map) {
>+		if (pool->dma_sync) {
>+			/* paired with READ_ONCE in
>+			 * page_pool_dma_sync_for_device() and
>+			 * __page_pool_dma_sync_for_cpu()
>+			 */
>+			WRITE_ONCE(pool->dma_sync, false);
>+
>+			/* Make sure all concurrent returns that may see the old
>+			 * value of dma_sync (and thus perform a sync) have
>+			 * finished before doing the unmapping below. Skip the
>+			 * wait if the device doesn't actually need syncing, or
>+			 * if there are no outstanding mapped pages.
>+			 */
>+			if (dma_dev_need_sync(pool->p.dev) &&
>+			    !xa_empty(&pool->dma_mapped))
>+				synchronize_net();
>+		}
>+
>+		xa_for_each(&pool->dma_mapped, id, ptr)
>+			__page_pool_release_page_dma(pool, page_to_netmem(ptr));
>+	}
>
> 	/* No more consumers should exist, but producers could still
> 	 * be in-flight.
>
>-- 
>2.48.1
>

Mina Almasry March 26, 2025, 8:02 p.m. UTC | #3

On Wed, Mar 26, 2025 at 11:22 AM Saeed Mahameed <[e-mail]> wrote:
>
> On 25 Mar 16:45, Toke Høiland-Jørgensen wrote:
> >When enabling DMA mapping in page_pool, pages are kept DMA mapped until
> >they are released from the pool, to avoid the overhead of re-mapping the
> >pages every time they are used. This causes resource leaks and/or
> >crashes when there are pages still outstanding while the device is torn
> >down, because page_pool will attempt an unmap through a non-existent DMA
> >device on the subsequent page return.
> >
>
> Why dynamically track when it is guaranteed the page_pool consumer (driver)
> will return all outstanding pages before disabling the DMA device.
> When a page pool is destroyed by the driver, just mark it as "DMA-inactive",
> and on page_pool_return_page() if DMA-inactive don't recycle those pages
> and immediately DMA unmap and release them.

That doesn't work, AFAIU. DMA unmaping after page_pool_destroy has
been called in what's causing the very bug this series is trying to
fix. What happens is:

1. Driver calls page_pool_destroy,
2. Driver removes the net_device (and I guess the associated iommu
structs go away with it).
3. Page-pool tries to unmap after page_pool_destroy is called, trying
to fetch iommu resources that have been freed due to the netdevice
gone away = bad stuff.

(but maybe I misunderstood your suggestion)

Saeed Mahameed March 27, 2025, 12:29 a.m. UTC | #4

On 26 Mar 13:02, Mina Almasry wrote:
>On Wed, Mar 26, 2025 at 11:22 AM Saeed Mahameed <[e-mail]> wrote:
>>
>> On 25 Mar 16:45, Toke Høiland-Jørgensen wrote:
>> >When enabling DMA mapping in page_pool, pages are kept DMA mapped until
>> >they are released from the pool, to avoid the overhead of re-mapping the
>> >pages every time they are used. This causes resource leaks and/or
>> >crashes when there are pages still outstanding while the device is torn
>> >down, because page_pool will attempt an unmap through a non-existent DMA
>> >device on the subsequent page return.
>> >
>>
>> Why dynamically track when it is guaranteed the page_pool consumer (driver)
>> will return all outstanding pages before disabling the DMA device.
>> When a page pool is destroyed by the driver, just mark it as "DMA-inactive",
>> and on page_pool_return_page() if DMA-inactive don't recycle those pages
>> and immediately DMA unmap and release them.
>
>That doesn't work, AFAIU. DMA unmaping after page_pool_destroy has
>been called in what's causing the very bug this series is trying to
>fix. What happens is:
>
>1. Driver calls page_pool_destroy,

Here the driver should already have returned all inflight pages to the
pool, the problem is that those returned pages are recycled back, instead
of being dma unmapped, all we need to do is to mark page_pool as "don't
recycle" after the driver destroyed it.

>2. Driver removes the net_device (and I guess the associated iommu
>structs go away with it).

if the driver had not yet released those pages at this point then there is a
more series leak than just dma leak. If the pool has those pages, which is
probably the case, then they were already release by the driver, the problem
is that they were recycled back.

>3. Page-pool tries to unmap after page_pool_destroy is called, trying
>to fetch iommu resources that have been freed due to the netdevice
>gone away = bad stuff.
>
>(but maybe I misunderstood your suggestion)

Yes, see above, but I just double checked the code and I though that the
page_pool_destroy would wait for all inflight pages to be release before it
returns back to the caller, but apparently I was mistaken, if the pages are
still being held by stack/user-space then they will still be considered
"inflight" although the driver is done with them :/.

So yes tracking "ALL" pages is one way to solve it, but I still think that
the correct way to deal with this is to hold the driver/netdev while there
are inflight pages, but no strong opinion if we expect pages to remain in
userspace for "too" long, then I admit, tracking is the only way.

>
>-- 
>Thanks,
>Mina

Mina Almasry March 27, 2025, 1:37 a.m. UTC | #5

On Wed, Mar 26, 2025 at 5:29 PM Saeed Mahameed <[e-mail]> wrote:
>
> On 26 Mar 13:02, Mina Almasry wrote:
> >On Wed, Mar 26, 2025 at 11:22 AM Saeed Mahameed <[e-mail]> wrote:
> >>
> >> On 25 Mar 16:45, Toke Høiland-Jørgensen wrote:
> >> >When enabling DMA mapping in page_pool, pages are kept DMA mapped until
> >> >they are released from the pool, to avoid the overhead of re-mapping the
> >> >pages every time they are used. This causes resource leaks and/or
> >> >crashes when there are pages still outstanding while the device is torn
> >> >down, because page_pool will attempt an unmap through a non-existent DMA
> >> >device on the subsequent page return.
> >> >
> >>
> >> Why dynamically track when it is guaranteed the page_pool consumer (driver)
> >> will return all outstanding pages before disabling the DMA device.
> >> When a page pool is destroyed by the driver, just mark it as "DMA-inactive",
> >> and on page_pool_return_page() if DMA-inactive don't recycle those pages
> >> and immediately DMA unmap and release them.
> >
> >That doesn't work, AFAIU. DMA unmaping after page_pool_destroy has
> >been called in what's causing the very bug this series is trying to
> >fix. What happens is:
> >
> >1. Driver calls page_pool_destroy,
>
> Here the driver should already have returned all inflight pages to the
> pool, the problem is that those returned pages are recycled back, instead
> of being dma unmapped, all we need to do is to mark page_pool as "don't
> recycle" after the driver destroyed it.
>
> >2. Driver removes the net_device (and I guess the associated iommu
> >structs go away with it).
>
> if the driver had not yet released those pages at this point then there is a
> more series leak than just dma leak. If the pool has those pages, which is
> probably the case, then they were already release by the driver, the problem
> is that they were recycled back.
>
> >3. Page-pool tries to unmap after page_pool_destroy is called, trying
> >to fetch iommu resources that have been freed due to the netdevice
> >gone away = bad stuff.
> >
> >(but maybe I misunderstood your suggestion)
>
> Yes, see above, but I just double checked the code and I though that the
> page_pool_destroy would wait for all inflight pages to be release before it
> returns back to the caller, but apparently I was mistaken, if the pages are
> still being held by stack/user-space then they will still be considered
> "inflight" although the driver is done with them :/.
>

Right, I think we're on the same page now. page_pool_destroy doesn't
(currently) wait for all the inflight pages to be returned before it
returns to the driver calling it, even if the driver is fully done
with all the pages. There could be pages still held by the
userspace/net stack.

> So yes tracking "ALL" pages is one way to solve it, but I still think that
> the correct way to deal with this is to hold the driver/netdev while there
> are inflight pages, but no strong opinion if we expect pages to remain in
> userspace for "too" long, then I admit, tracking is the only way.
>

AFAICT, the amount of time the userspace can hold onto an inflight
page is actually indefinite. The pathological edge case is the
userspace opens a receive socket, never closes it, and never
recvmsg()'s it. In that case skbs will wait in the socket's receive
queue forever.

FWIW Jakub did suggest a fix where the page_pool will stall the
netdevice removal while there are inflight pages. I never provided
Reviewed-by because I was nervous about GVE failing to soft reset or
unload or something because some userspace is holding onto a page_pool
page, but maybe you like it better:

https://lore.kernel.org/netdev/[e-mail]/T/#mbca7f9391ba44840444e747c9038ef6617142048

My personal feeling is that adding overhead to the slow page_alloc +
dma mapping path is preferable to blocking netdev unregister.

Yunsheng Lin March 27, 2025, 3:53 a.m. UTC | #6

On 2025/3/27 2:22, Saeed Mahameed wrote:

...

> 
> I know there's an extra check on fast path, but looking below it seems
> like slow path is becoming unnecessarily complicated, and we are sacrificing
> slow path performance significantly for the sake of not touching fast path
> at all. page_pool slow path should _not_ be significantly slower
> than using the page allocator directly! In many production environments and
> some workloads, page pool recycling can happen at a very low rate resulting
> on performance relying solely on slow path..

+1. And we also rely on the above 'slow path' to flush old nid-mismatched pages
and refill new pages from the correct nid, see the nid checking and updating in
page_pool_refill_alloc_cache() and page_pool_update_nid() when irq migrates
between different numa nodes as it seems common for linux distributions to use
irqbalance to tune performance.

>> To fix this, implement a simple tracking of outstanding DMA-mapped pages
>> in page pool using an xarray. This was first suggested by Mina[0], and
>> turns out to be fairly straight forward: We simply store pointers to
>> pages directly in the xarray with xa_alloc() when they are first DMA
>> mapped, and remove them from the array on unmap. Then, when a page pool
>> is torn down, it can simply walk the xarray and unmap all pages still
>> present there before returning, which also allows us to get rid of the
>> get/put_device() calls in page_pool. Using xa_cmpxchg(), no additional
>> synchronisation is needed, as a page will only ever be unmapped once.
>>
>> To avoid having to walk the entire xarray on unmap to find the page
>> reference, we stash the ID assigned by xa_alloc() into the page
>> structure itself, using the upper bits of the pp_magic field. This
>> requires a couple of defines to avoid conflicting with the
>> POINTER_POISON_DELTA define, but this is all evaluated at compile-time,
>> so does not affect run-time performance. The bitmap calculations in this
>> patch gives the following number of bits for different architectures:
>>
>> - 23 bits on 32-bit architectures
>> - 21 bits on PPC64 (because of the definition of ILLEGAL_POINTER_VALUE)
>> - 32 bits on other 64-bit architectures
>>
>> Stashing a value into the unused bits of pp_magic does have the effect
>> that it can make the value stored there lie outside the unmappable
>> range (as governed by the mmap_min_addr sysctl), for architectures that
>> don't define ILLEGAL_POINTER_VALUE. This means that if one of the
>> pointers that is aliased to the pp_magic field (such as page->lru.next)
>> is dereferenced while the page is owned by page_pool, that could lead to
>> a dereference into userspace, which is a security concern. The risk of
>> this is mitigated by the fact that (a) we always clear pp_magic before
>> releasing a page from page_pool, and (b) this would need a
>> use-after-free bug for struct page, which can have many other risks
>> since page->lru.next is used as a generic list pointer in multiple
>> places in the kernel. As such, with this patch we take the position that
>> this risk is negligible in practice. For more discussion, see[1].

The below is a recent UAF for a page, I guess it can be said that this
patch seem to basically make mmap_min_addr mechanism useless for the above
arches when a driver with page_pool dma mapping support is loaded:
https://lore.kernel.org/all/[e-mail]/

So how about logging a warning so that user can tell if their system may
be in security compromised state for the above case?

>>
>> Since all the tracking added in this patch is performed on DMA
>> map/unmap, no additional code is needed in the fast path, meaning the
>> performance overhead of this tracking is negligible there. A
>> micro-benchmark shows that the total overhead of the tracking itself is
>> about 400 ns (39 cycles(tsc) 395.218 ns; sum for both map and unmap[2]).
>> Since this cost is only paid on DMA map and unmap, it seems like an
>> acceptable cost to fix the late unmap issue. Further optimisation can
>> narrow the cases where this cost is paid (for instance by eliding the
>> tracking when DMA map/unmap is a no-op).
>>
> What I am missing here, what is the added cost of those extra operations on
> the slow path compared to before this patch? Total overhead being
> acceptable doesn't justify the change, we need diff before and after.

Toke used my data in [2] below:
The above 400ns is the added cost of those extra operations on the slow path,
before this patch the slow path only cost about 170ns, so there is more than
200% performance degradation for the page tracking in this patch, which I
failed to see why it is acceptable:(

> 
>> The extra memory needed to track the pages is neatly encapsulated inside
>> xarray, which uses the 'struct xa_node' structure to track items. This
>> structure is 576 bytes long, with slots for 64 items, meaning that a
>> full node occurs only 9 bytes of overhead per slot it tracks (in
>> practice, it probably won't be this efficient, but in any case it should
>> be an acceptable overhead).
>>
>> [0] https://lore.kernel.org/all/[e-mail]/
>> [1] https://lore.kernel.org/r/[e-mail]
>> [2] https://lore.kernel.org/r/[e-mail]

Mina Almasry March 27, 2025, 4:59 a.m. UTC | #7

On Wed, Mar 26, 2025 at 8:54 PM Yunsheng Lin <[e-mail]> wrote:
> >>
> >> Since all the tracking added in this patch is performed on DMA
> >> map/unmap, no additional code is needed in the fast path, meaning the
> >> performance overhead of this tracking is negligible there. A
> >> micro-benchmark shows that the total overhead of the tracking itself is
> >> about 400 ns (39 cycles(tsc) 395.218 ns; sum for both map and unmap[2]).
> >> Since this cost is only paid on DMA map and unmap, it seems like an
> >> acceptable cost to fix the late unmap issue. Further optimisation can
> >> narrow the cases where this cost is paid (for instance by eliding the
> >> tracking when DMA map/unmap is a no-op).
> >>
> > What I am missing here, what is the added cost of those extra operations on
> > the slow path compared to before this patch? Total overhead being
> > acceptable doesn't justify the change, we need diff before and after.
>
> Toke used my data in [2] below:
> The above 400ns is the added cost of those extra operations on the slow path,
> before this patch the slow path only cost about 170ns, so there is more than
> 200% performance degradation for the page tracking in this patch, which I
> failed to see why it is acceptable:(
>

You may be correct about the absolute value of the overhead added
(400ns), I'm not sure it's a 200% regression though.

what time_bench_page_pool03_slow actually does each iteration:
- Allocates a page *from the fast path*
- Frees a page to through the slow path (recycling disabled).

Notably it doesn't do anything in the slow path that I imagine is
actually expensive: alloc_page, dma_map_page, & dma_unmap_page.

We do not have an existing benchmark case that actually tests the full
cost of the slow path (i.e full cost of page_pool_alloc from slow path
with dma-mapping and page_pool_put_page to the slow path with
dma-unmapping). That test case would have given us the full picture in
terms of % regression.

This is partly why I want to upstream the benchmark. Such cases can be
added after it is upstreamed.

--
Thanks,
Mina

Yunsheng Lin March 27, 2025, 7:21 a.m. UTC | #8

On 2025/3/27 12:59, Mina Almasry wrote:
> On Wed, Mar 26, 2025 at 8:54 PM Yunsheng Lin <[e-mail]> wrote:
>>>>
>>>> Since all the tracking added in this patch is performed on DMA
>>>> map/unmap, no additional code is needed in the fast path, meaning the
>>>> performance overhead of this tracking is negligible there. A
>>>> micro-benchmark shows that the total overhead of the tracking itself is
>>>> about 400 ns (39 cycles(tsc) 395.218 ns; sum for both map and unmap[2]).
>>>> Since this cost is only paid on DMA map and unmap, it seems like an
>>>> acceptable cost to fix the late unmap issue. Further optimisation can
>>>> narrow the cases where this cost is paid (for instance by eliding the
>>>> tracking when DMA map/unmap is a no-op).

See the above statement, and note the above optimisation was also discussed
before and it seemed unfeasible too.

> 
> what time_bench_page_pool03_slow actually does each iteration:
> - Allocates a page *from the fast path*
> - Frees a page to through the slow path (recycling disabled).
> 
> Notably it doesn't do anything in the slow path that I imagine is
> actually expensive: alloc_page, dma_map_page, & dma_unmap_page.

As above, for most arches, the DMA map/unmap seems to be almost no-op when
page_pool is created with PP_FLAG_DMA_MAP flag without IOMMU/swiotlb behind
the DMA MAPPING.

> 
> We do not have an existing benchmark case that actually tests the full
> cost of the slow path (i.e full cost of page_pool_alloc from slow path
> with dma-mapping and page_pool_put_page to the slow path with
> dma-unmapping). That test case would have given us the full picture in
> terms of % regression.
> 
> This is partly why I want to upstream the benchmark. Such cases can be
> added after it is upstreamed.

Why not add it now when you seemed to be arguing that exercising the code
path of dma_map_page() and dma_unmap_page() may change the full picture
here.

Patch

diff --git a/include/linux/poison.h b/include/linux/poison.h
index 331a9a996fa8746626afa63ea462b85ca3e5938b..5351efd710d5e21cc341f7bb533b1aeea4a0808a 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -70,6 +70,10 @@ 
 #define KEY_DESTROY		0xbd
 
 /********** net/core/page_pool.c **********/
+/*
+ * page_pool uses additional free bits within this value to store data, see the
+ * definition of PP_DMA_INDEX_MASK in include/net/page_pool/types.h
+ */
 #define PP_SIGNATURE		(0x40 + POISON_POINTER_DELTA)
 
 /********** net/core/skbuff.c **********/
diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index fbe34024b20061e8bcd1d4474f6ebfc70992f1eb..8f9ed92a4b2af6c136406c41b1a829c8e52ba3e2 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -6,6 +6,7 @@ 
 #include <linux/dma-direction.h>
 #include <linux/ptr_ring.h>
 #include <linux/types.h>
+#include <linux/xarray.h>
 #include <net/netmem.h>
 
 #define PP_FLAG_DMA_MAP		BIT(0) /* Should page_pool do the DMA
@@ -58,13 +59,51 @@  struct pp_alloc_cache {
 	netmem_ref cache[PP_ALLOC_CACHE_SIZE];
 };
 
+/*
+ * DMA mapping IDs
+ *
+ * When DMA-mapping a page, we allocate an ID (from an xarray) and stash this in
+ * the upper bits of page->pp_magic. We always want to be able to unambiguously
+ * identify page pool pages (using page_pool_page_is_pp()). Non-PP pages can
+ * have arbitrary kernel pointers stored in the same field as pp_magic (since it
+ * overlaps with page->lru.next), so we must ensure that we cannot mistake a
+ * valid kernel pointer with any of the values we write into this field.
+ *
+ * On architectures that set POISON_POINTER_DELTA, this is already ensured,
+ * since this value becomes part of PP_SIGNATURE; meaning we can just use the
+ * space between the PP_SIGNATURE value (without POISON_POINTER_DELTA), and the
+ * lowest bits of POISON_POINTER_DELTA. On arches where POISON_POINTER_DELTA is
+ * 0, we make sure that we leave the two topmost bits empty, as that guarantees
+ * we won't mistake a valid kernel pointer for a value we set, regardless of the
+ * VMSPLIT setting.
+ *
+ * Altogether, this means that the number of bits available is constrained by
+ * the size of an unsigned long (at the upper end, subtracting two bits per the
+ * above), and the definition of PP_SIGNATURE (with or without
+ * POISON_POINTER_DELTA).
+ */
+#define PP_DMA_INDEX_SHIFT (1 + __fls(PP_SIGNATURE - POISON_POINTER_DELTA))
+#if POISON_POINTER_DELTA > 0
+/* PP_SIGNATURE includes POISON_POINTER_DELTA, so limit the size of the DMA
+ * index to not overlap with that if set
+ */
+#define PP_DMA_INDEX_BITS MIN(32, __ffs(POISON_POINTER_DELTA) - PP_DMA_INDEX_SHIFT)
+#else
+/* Always leave out the topmost two; see above. */
+#define PP_DMA_INDEX_BITS MIN(32, BITS_PER_LONG - PP_DMA_INDEX_SHIFT - 2)
+#endif
+
+#define PP_DMA_INDEX_MASK GENMASK(PP_DMA_INDEX_BITS + PP_DMA_INDEX_SHIFT - 1, \
+				  PP_DMA_INDEX_SHIFT)
+#define PP_DMA_INDEX_LIMIT XA_LIMIT(1, BIT(PP_DMA_INDEX_BITS) - 1)
+
 /* Mask used for checking in page_pool_page_is_pp() below. page->pp_magic is
  * OR'ed with PP_SIGNATURE after the allocation in order to preserve bit 0 for
- * the head page of compound page and bit 1 for pfmemalloc page.
- * page_is_pfmemalloc() is checked in __page_pool_put_page() to avoid recycling
- * the pfmemalloc page.
+ * the head page of compound page and bit 1 for pfmemalloc page, as well as the
+ * bits used for the DMA index. page_is_pfmemalloc() is checked in
+ * __page_pool_put_page() to avoid recycling the pfmemalloc page.
  */
-#define PP_MAGIC_MASK ~0x3UL
+#define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
 
 /**
  * struct page_pool_params - page pool parameters
@@ -233,6 +272,8 @@  struct page_pool {
 	void *mp_priv;
 	const struct memory_provider_ops *mp_ops;
 
+	struct xarray dma_mapped;
+
 #ifdef CONFIG_PAGE_POOL_STATS
 	/* recycle stats are per-cpu to avoid locking */
 	struct page_pool_recycle_stats __percpu *recycle_stats;
diff --git a/net/core/netmem_priv.h b/net/core/netmem_priv.h
index f33162fd281c23e109273ba09950c5d0a2829bc9..cd95394399b40c3604934ba7898eeeeacb8aee99 100644
--- a/net/core/netmem_priv.h
+++ b/net/core/netmem_priv.h
@@ -5,7 +5,7 @@ 
 
 static inline unsigned long netmem_get_pp_magic(netmem_ref netmem)
 {
-	return __netmem_clear_lsb(netmem)->pp_magic;
+	return __netmem_clear_lsb(netmem)->pp_magic & ~PP_DMA_INDEX_MASK;
 }
 
 static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic)
@@ -15,6 +15,8 @@  static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic)
 
 static inline void netmem_clear_pp_magic(netmem_ref netmem)
 {
+	WARN_ON_ONCE(__netmem_clear_lsb(netmem)->pp_magic & PP_DMA_INDEX_MASK);
+
 	__netmem_clear_lsb(netmem)->pp_magic = 0;
 }
 
@@ -33,4 +35,28 @@  static inline void netmem_set_dma_addr(netmem_ref netmem,
 {
 	__netmem_clear_lsb(netmem)->dma_addr = dma_addr;
 }
+
+static inline unsigned long netmem_get_dma_index(netmem_ref netmem)
+{
+	unsigned long magic;
+
+	if (WARN_ON_ONCE(netmem_is_net_iov(netmem)))
+		return 0;
+
+	magic = __netmem_clear_lsb(netmem)->pp_magic;
+
+	return (magic & PP_DMA_INDEX_MASK) >> PP_DMA_INDEX_SHIFT;
+}
+
+static inline void netmem_set_dma_index(netmem_ref netmem,
+					unsigned long id)
+{
+	unsigned long magic;
+
+	if (WARN_ON_ONCE(netmem_is_net_iov(netmem)))
+		return;
+
+	magic = netmem_get_pp_magic(netmem) | (id << PP_DMA_INDEX_SHIFT);
+	__netmem_clear_lsb(netmem)->pp_magic = magic;
+}
 #endif
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index d51ca4389dd62d8bc266a9a2b792838257173535..55acb4bc2c57893486f222e9f39b48a09b0d78d0 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -226,6 +226,8 @@  static int page_pool_init(struct page_pool *pool,
 			return -EINVAL;
 
 		pool->dma_map = true;
+
+		xa_init_flags(&pool->dma_mapped, XA_FLAGS_ALLOC1);
 	}
 
 	if (pool->slow.flags & PP_FLAG_DMA_SYNC_DEV) {
@@ -275,9 +277,6 @@  static int page_pool_init(struct page_pool *pool,
 	/* Driver calling page_pool_create() also call page_pool_destroy() */
 	refcount_set(&pool->user_cnt, 1);
 
-	if (pool->dma_map)
-		get_device(pool->p.dev);
-
 	if (pool->slow.flags & PP_FLAG_ALLOW_UNREADABLE_NETMEM) {
 		/* We rely on rtnl_lock()ing to make sure netdev_rx_queue
 		 * configuration doesn't change while we're initializing
@@ -325,7 +324,7 @@  static void page_pool_uninit(struct page_pool *pool)
 	ptr_ring_cleanup(&pool->ring, NULL);
 
 	if (pool->dma_map)
-		put_device(pool->p.dev);
+		xa_destroy(&pool->dma_mapped);
 
 #ifdef CONFIG_PAGE_POOL_STATS
 	if (!pool->system)
@@ -466,14 +465,20 @@  page_pool_dma_sync_for_device(const struct page_pool *pool,
 			      netmem_ref netmem,
 			      u32 dma_sync_size)
 {
-	if ((READ_ONCE(pool->dma_sync) & PP_DMA_SYNC_DEV) &&
-	    dma_dev_need_sync(pool->p.dev))
-		__page_pool_dma_sync_for_device(pool, netmem, dma_sync_size);
+	if (dma_dev_need_sync(pool->p.dev)) {
+		rcu_read_lock();
+		if (READ_ONCE(pool->dma_sync) & PP_DMA_SYNC_DEV)
+			__page_pool_dma_sync_for_device(pool, netmem,
+							dma_sync_size);
+		rcu_read_unlock();
+	}
 }
 
-static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
+static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t gfp)
 {
 	dma_addr_t dma;
+	int err;
+	u32 id;
 
 	/* Setup DMA mapping: use 'struct page' area for storing DMA-addr
 	 * since dma_addr_t can be either 32 or 64 bits and does not always fit
@@ -487,15 +492,28 @@  static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
 	if (dma_mapping_error(pool->p.dev, dma))
 		return false;
 
-	if (page_pool_set_dma_addr_netmem(netmem, dma))
+	if (in_softirq())
+		err = xa_alloc(&pool->dma_mapped, &id, netmem_to_page(netmem),
+			       PP_DMA_INDEX_LIMIT, gfp);
+	else
+		err = xa_alloc_bh(&pool->dma_mapped, &id, netmem_to_page(netmem),
+				  PP_DMA_INDEX_LIMIT, gfp);
+	if (err) {
+		WARN_ONCE(1, "couldn't track DMA mapping, please report to netdev@");
 		goto unmap_failed;
+	}
 
+	if (page_pool_set_dma_addr_netmem(netmem, dma)) {
+		WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
+		goto unmap_failed;
+	}
+
+	netmem_set_dma_index(netmem, id);
 	page_pool_dma_sync_for_device(pool, netmem, pool->p.max_len);
 
 	return true;
 
 unmap_failed:
-	WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
 	dma_unmap_page_attrs(pool->p.dev, dma,
 			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
 			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
@@ -512,7 +530,7 @@  static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
 	if (unlikely(!page))
 		return NULL;
 
-	if (pool->dma_map && unlikely(!page_pool_dma_map(pool, page_to_netmem(page)))) {
+	if (pool->dma_map && unlikely(!page_pool_dma_map(pool, page_to_netmem(page), gfp))) {
 		put_page(page);
 		return NULL;
 	}
@@ -558,7 +576,7 @@  static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
 	 */
 	for (i = 0; i < nr_pages; i++) {
 		netmem = pool->alloc.cache[i];
-		if (dma_map && unlikely(!page_pool_dma_map(pool, netmem))) {
+		if (dma_map && unlikely(!page_pool_dma_map(pool, netmem, gfp))) {
 			put_page(netmem_to_page(netmem));
 			continue;
 		}
@@ -660,6 +678,8 @@  void page_pool_clear_pp_info(netmem_ref netmem)
 static __always_inline void __page_pool_release_page_dma(struct page_pool *pool,
 							 netmem_ref netmem)
 {
+	struct page *old, *page = netmem_to_page(netmem);
+	unsigned long id;
 	dma_addr_t dma;
 
 	if (!pool->dma_map)
@@ -668,6 +688,17 @@  static __always_inline void __page_pool_release_page_dma(struct page_pool *pool,
 		 */
 		return;
 
+	id = netmem_get_dma_index(netmem);
+	if (!id)
+		return;
+
+	if (in_softirq())
+		old = xa_cmpxchg(&pool->dma_mapped, id, page, NULL, 0);
+	else
+		old = xa_cmpxchg_bh(&pool->dma_mapped, id, page, NULL, 0);
+	if (old != page)
+		return;
+
 	dma = page_pool_get_dma_addr_netmem(netmem);
 
 	/* When page is unmapped, it cannot be returned to our pool */
@@ -675,6 +706,7 @@  static __always_inline void __page_pool_release_page_dma(struct page_pool *pool,
 			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
 			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
 	page_pool_set_dma_addr_netmem(netmem, 0);
+	netmem_set_dma_index(netmem, 0);
 }
 
 /* Disconnects a page (from a page_pool).  API users can have a need
@@ -1084,8 +1116,32 @@  static void page_pool_empty_alloc_cache_once(struct page_pool *pool)
 
 static void page_pool_scrub(struct page_pool *pool)
 {
+	unsigned long id;
+	void *ptr;
+
 	page_pool_empty_alloc_cache_once(pool);
-	pool->destroy_cnt++;
+	if (!pool->destroy_cnt++ && pool->dma_map) {
+		if (pool->dma_sync) {
+			/* paired with READ_ONCE in
+			 * page_pool_dma_sync_for_device() and
+			 * __page_pool_dma_sync_for_cpu()
+			 */
+			WRITE_ONCE(pool->dma_sync, false);
+
+			/* Make sure all concurrent returns that may see the old
+			 * value of dma_sync (and thus perform a sync) have
+			 * finished before doing the unmapping below. Skip the
+			 * wait if the device doesn't actually need syncing, or
+			 * if there are no outstanding mapped pages.
+			 */
+			if (dma_dev_need_sync(pool->p.dev) &&
+			    !xa_empty(&pool->dma_mapped))
+				synchronize_net();
+		}
+
+		xa_for_each(&pool->dma_mapped, id, ptr)
+			__page_pool_release_page_dma(pool, page_to_netmem(ptr));
+	}
 
 	/* No more consumers should exist, but producers could still
 	 * be in-flight.