Re: [PATCH net v2 3/4] netdevsim: support devmem RX and TX bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] netmem: devmem leak and kernel warning in nsim_pp_hold_write()
--

commit b58e0da7daeddfad13177ed902471523d6b96cbf
Author: Mina Almasry <almasrymina@google.com>

netdevsim: support devmem RX and TX bindings

Set a 64-bit DMA mask on nsim_bus_dev, enable NETMEM_TX_DMA on the
netdevsim net_device, and configure page_pool instances with
PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | PP_FLAG_ALLOW_UNREADABLE_NETMEM
and queue_idx so devmem bind-rx and bind-tx netlink operations can be
tested against netdevsim in software selftests.

> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index b4a99f3ceac60..4415dc01c610b 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c

[ ... ]

> @@ -414,16 +414,22 @@ static int nsim_poll(struct napi_struct *napi, int budget)
>  	return done;
>  }
>  
> -static int nsim_create_page_pool(struct page_pool **p, struct napi_struct *napi)
> +static int nsim_create_page_pool(struct net_device *dev, struct page_pool **p,
> +				 struct napi_struct *napi, int idx)
>  {
> +	struct netdevsim *ns = netdev_priv(dev);
>  	struct page_pool_params params = {
>  		.order = 0,
>  		.pool_size = NSIM_RING_SIZE,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV |
> +			 PP_FLAG_ALLOW_UNREADABLE_NETMEM,

[Severity: Medium]
When PP_FLAG_ALLOW_UNREADABLE_NETMEM is set and devmem is bound, will this
cause a leak and kernel warning in the debugfs handler?

If nsim_pp_hold_write() is called to test allocating memory from the pool,
it uses page_pool_dev_alloc_pages():

drivers/net/netdevsim/netdev.c:nsim_pp_hold_write() {
    ...
	} else if (val) {
		ns->page = page_pool_dev_alloc_pages(ns->rq[0]->page_pool);
		if (!ns->page)
			ret = -ENOMEM;
    ...
}

Because devmem may be bound, page_pool_alloc_netmems() can allocate an
unreadable net_iov. When this is passed to netmem_to_page(), it detects an
IOV, triggers a WARN_ON_ONCE(), and returns NULL.

nsim_pp_hold_write() receives NULL and returns -ENOMEM, leaking the
allocated devmem netmem_ref because it is neither stored nor freed. 

While this is restricted to root via debugfs, does this break the exact
devmem testing functionality this patch intends to support?

> +		.max_len = PAGE_SIZE,
>  		.nid = NUMA_NO_NODE,
> -		.dev = &napi->dev->dev,
> +		.dev = &ns->nsim_bus_dev->dev,
>  		.napi = napi,
>  		.dma_dir = DMA_BIDIRECTIONAL,
> -		.netdev = napi->dev,
> +		.netdev = dev,
> +		.queue_idx = idx,
>  	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012174300.2338784-1-almasrymina@google.com?part=3