Re: [RFC PATCH net-next 02/13] net: devmem: extend memory provider for knod

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The patch silently increases PP_ALLOC_CACHE_REFILL from 64 to up to 4096 (for 4K pages), severely bloating struct page_pool and causing order-5 page allocation failures.
- [High] Ignored error return value of page_pool_set_dma_addr_netmem can lead to silent DMA address truncation and memory corruption on 32-bit architectures with 64-bit DMA.
- [Medium] The is_kernel_rodata check in page_pool_init prevents loadable driver modules from utilizing the new PP_FLAG_CUSTOM_MEMORY_PROVIDER feature.
--

--- Patch [2]: [RFC PATCH net-next 02/13] net: devmem: extend memory provider for knod ---
commit 685f164a3b04db75b3dba6257fd2806218155a24
Author: Taehee Yoo <ap420073@gmail.com>

net: devmem: extend memory provider for knod

This commit extends the devmem memory-provider path so a knod accelerator
can back a NIC page_pool with accelerator-exported memory. This lets the
NIC DMA received packets directly into accelerator memory.

> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
> index 03da138722f58..3e866f249768e 100644
> --- a/include/net/page_pool/types.h
> +++ b/include/net/page_pool/types.h
[ ... ]
> @@ -54,11 +62,11 @@
>   * would have to take a slower code path.
>   */
>  #if PAGE_SIZE >= SZ_64K
> -#define PP_ALLOC_CACHE_REFILL	4
> +#define PP_ALLOC_CACHE_REFILL	256
>  #elif PAGE_SIZE >= SZ_16K
> -#define PP_ALLOC_CACHE_REFILL	16
> +#define PP_ALLOC_CACHE_REFILL	1024
>  #else
> -#define PP_ALLOC_CACHE_REFILL	64
> +#define PP_ALLOC_CACHE_REFILL	4096
>  #endif

[Severity: High]
Will this increase to PP_ALLOC_CACHE_REFILL cause page_pool_create() to
fail unpredictably under memory fragmentation?

Since PP_ALLOC_CACHE_SIZE is defined as PP_ALLOC_CACHE_REFILL * 2, the
inline cache array inside struct page_pool grows to 8,192 entries for 4K
pages. At 8 bytes per pointer, this array alone consumes ~65.5KB.

This forces an order-5 kzalloc_node allocation for the page_pool struct,
which is highly prone to failure under fragmentation and drastically bloats
the memory footprint per RX queue.

[ ... ]
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 21dc4a9c8714d..fd7444943b9f5 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -272,7 +272,17 @@ static int page_pool_init(struct page_pool *pool,
>  
>  	xa_init_flags(&pool->dma_mapped, XA_FLAGS_ALLOC1);
>  
> -	if (pool->slow.flags & PP_FLAG_ALLOW_UNREADABLE_NETMEM) {
> +	if (pool->slow.flags & PP_FLAG_CUSTOM_MEMORY_PROVIDER) {
> +		/* Driver-managed pool with a directly-supplied memory
> +		 * provider, not bound to a netdev rx queue.
> +		 */
> +		if (WARN_ON(!pool->slow.mp_ops || !pool->slow.mp_priv)) {
> +			err = -EINVAL;
> +			goto free_ptr_ring;
> +		}
> +		pool->mp_priv = pool->slow.mp_priv;
> +		pool->mp_ops = pool->slow.mp_ops;

[Severity: Medium]
Will this new flag work for loadable driver modules?

When PP_FLAG_CUSTOM_MEMORY_PROVIDER is used, pool->mp_ops is populated here.
Later in page_pool_init(), there is a preexisting check:

    if (WARN_ON(!is_kernel_rodata((unsigned long)pool->mp_ops))) {
        err = -EFAULT;
        goto free_ptr_ring;
    }

Because is_kernel_rodata() only checks against the core kernel's
__start_rodata and __end_rodata boundaries, any memory provider ops struct
allocated in a loadable module's .rodata section will fail this check.
This renders the new feature unusable for modular drivers.

> +	} else if (pool->slow.flags & PP_FLAG_ALLOW_UNREADABLE_NETMEM) {
>  		netdev_assert_locked(pool->slow.netdev);
>  		rxq = __netif_get_rx_queue(pool->slow.netdev,
>  					   pool->slow.queue_idx);
[ ... ]
> @@ -725,6 +735,16 @@ void page_pool_clear_pp_info(netmem_ref netmem)
>  	netmem_set_pp(netmem, NULL);
>  }
>  
> +void page_pool_provider_set_netmem(struct page_pool *pool, netmem_ref netmem,
> +				   dma_addr_t addr)
> +{
> +	netmem_to_nmdesc(netmem)->pp_magic = 0;
> +	netmem_to_nmdesc(netmem)->pp = NULL;
> +	atomic_long_set(&netmem_to_nmdesc(netmem)->pp_ref_count, 0);
> +	page_pool_set_pp_info(pool, netmem);
> +	page_pool_set_dma_addr_netmem(netmem, addr);

[Severity: High]
Does this silently ignore DMA address truncation?

page_pool_set_dma_addr_netmem() returns a boolean indicating whether the
DMA address exceeded the capacity of the storage representation (such as
on 32-bit architectures with 64-bit DMA support).

Because page_pool_provider_set_netmem() returns void and discards this
boolean, memory providers will be unaware if their DMA address was
truncated. This could lead to silent data corruption or IOMMU faults
when the hardware uses the truncated address.

> +}
> +
>  static __always_inline void __page_pool_release_netmem_dma(struct page_pool *pool,
>  							   netmem_ref netmem)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719175857.4071636-1-ap420073@gmail.com?part=2