Re: [PATCH net-next v4 1/3] net: devmem: allow rx-buf-size > PAGE_SIZE per dmabuf binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Truncation of 64-bit `dma_addr_t` to 32-bit `unsigned long` when interacting with `gen_pool` causes memory leaks and corruption on 32-bit systems with LPAE.
--

commit 294321e299dc1978a803324d3c6d3e8d5fb93d2d
Author: Bobby Eshleman <bobbyeshleman@meta.com>

net: devmem: allow rx-buf-size > PAGE_SIZE per dmabuf binding

This commit introduces a new netlink attribute to allow devmem dmabuf
bindings to request a larger niov size. The value must be a power of two
greater than or equal to PAGE_SIZE, aiming to reduce CPU overhead for
large flows.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 957d6b96216b3..3d6cf35e50f30 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
[ ... ]
> @@ -113,12 +114,13 @@ void net_devmem_free_dmabuf(struct net_iov *niov)
>  {
>  	struct net_devmem_dmabuf_binding *binding = net_devmem_iov_binding(niov);
>  	unsigned long dma_addr = net_devmem_get_dma_addr(niov);
> +	size_t niov_size = 1UL << binding->niov_shift;
>  
>  	if (WARN_ON(!gen_pool_has_addr(binding->chunk_pool, dma_addr,
> -				       PAGE_SIZE)))
> +				       niov_size)))
>  		return;
>  
> -	gen_pool_free(binding->chunk_pool, dma_addr, PAGE_SIZE);
> +	gen_pool_free(binding->chunk_pool, dma_addr, niov_size);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does truncating dma_addr_t to
unsigned long in net_devmem_free_dmabuf() cause issues on 32-bit systems
with LPAE?

The net_devmem_get_dma_addr() function returns a 64-bit dma_addr_t, but it is
cast to an unsigned long local variable. The gen_pool_has_addr() and
gen_pool_free() functions also expect an unsigned long for the address.

If a DMA-BUF is mapped with a 64-bit dma_addr_t that exceeds the 32-bit
boundary, passing it as unsigned long will truncate it.

Could this lead to gen_pool_has_addr() failing and causing an unrecoverable
memory leak, or gen_pool_free() clearing bits in the wrong chunk and causing
memory corruption?

[ ... ]
> @@ -288,7 +295,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  
>  		owner->area.base_virtual = virtual;
>  		owner->base_dma_addr = dma_addr;
> -		owner->area.num_niovs = len / PAGE_SIZE;
> +		owner->area.num_niovs = len >> niov_shift;
>  		owner->binding = binding;
>  
>  		err = gen_pool_add_owner(binding->chunk_pool, dma_addr,

[Severity: High]
This is also a pre-existing issue, but does passing dma_addr to
gen_pool_add_owner() truncate the 64-bit DMA address here?

The virt parameter of gen_pool_add_owner() takes an unsigned long. If a
contiguous chunk crosses a 4GB boundary on a 32-bit LPAE system, would this
truncation cause a start_addr > end_addr wrap-around or overlap in the
gen_pool?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-tcpdm-large-niovs-v4-0-ca4654f37570@meta.com?part=1