Re: [PATCH net-next v3 2/3] net: devmem: embed net_iov_area in binding

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] net_devmem_bind_dmabuf() (net/core/devmem.c) sizes all three
  per-binding arrays from dmabuf->size (`binding->area.num_niovs =…

--

net: devmem: embed net_iov_area in binding

The chunk owner object is dropped and the single net_iov_area is stored
directly in struct net_devmem_dmabuf_binding.  net_devmem_iov_binding()
now derives the binding with container_of() on net_iov_owner(), and the
dmabuf_genpool_chunk_owner wrapper plus its alloc/free path are removed.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 2ac9d74010a9f..7cac0dab8ea87 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -52,12 +43,11 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
>  	struct net_devmem_dmabuf_binding *binding = container_of(wq, typeof(*binding), unbind_w);
>  
>  	if (binding->freelist)
> -		WARN(binding->free_count != binding->chunk_owner->area.num_niovs,
> +		WARN(binding->free_count != binding->area.num_niovs,
>  		     "destroying dmabuf binding with outstanding net_iovs: total=%zu, free=%zu",
> -		     binding->chunk_owner->area.num_niovs,
> -		     binding->free_count);
> +		     binding->area.num_niovs, binding->free_count);

[ ... ]

> @@ -241,16 +228,18 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  		goto err_unmap;
>  	}
>  
> -	total_niovs = dmabuf->size >> niov_shift;
> +	binding->area.base_virtual = 0;
> +	binding->area.num_niovs = dmabuf->size >> niov_shift;
>  	if (direction == DMA_TO_DEVICE) {
> -		binding->tx_vec = kvmalloc_objs(struct net_iov *, total_niovs);
> +		binding->tx_vec = kvmalloc_objs(struct net_iov *,
> +						binding->area.num_niovs);
>  		if (!binding->tx_vec) {
>  			err = -ENOMEM;
>  			goto err_unmap;
>  		}
>  	} else {
>  		spin_lock_init(&binding->freelist_lock);
> -		binding->freelist = kvmalloc_array(total_niovs,
> +		binding->freelist = kvmalloc_array(binding->area.num_niovs,
>  						   sizeof(binding->freelist[0]),
>  						   GFP_KERNEL);
>  		if (!binding->freelist) {

[ ... ]

> @@ -289,13 +267,13 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  		nr_niovs = len >> niov_shift;
>  		for (i = 0; i < nr_niovs; i++, niov_idx++) {
> -			niov = &owner->area.niovs[niov_idx];
> -			net_iov_init(niov, &owner->area, NET_IOV_DMABUF);
> +			niov = &binding->area.niovs[niov_idx];
> +			net_iov_init(niov, &binding->area, NET_IOV_DMABUF);
>  			page_pool_set_dma_addr_netmem(net_iov_to_netmem(niov),
>  						      dma_addr);
>  			if (direction == DMA_TO_DEVICE)

[Severity: Medium]
Can this loop in net_devmem_bind_dmabuf() write past
binding->area.niovs[], binding->freelist[] and binding->tx_vec[]?

All three arrays are now sized from the dmabuf size:

	binding->area.num_niovs = dmabuf->size >> niov_shift;

but the fill loop advances niov_idx (and binding->free_count) purely from
the mapped scatter-gather lengths, with no comparison against
binding->area.num_niovs:

	nr_niovs = len >> niov_shift;
	for (i = 0; i < nr_niovs; i++, niov_idx++) {
		niov = &binding->area.niovs[niov_idx];
		...
		if (direction == DMA_TO_DEVICE)
			binding->tx_vec[niov_idx] = niov;
		else
			binding->freelist[binding->free_count++] = niov;

The only per-entry check before this is
IS_ALIGNED(dma_addr, niov_size) / IS_ALIGNED(len, niov_size), which
constrains neither the total nor the index, and nothing sums
sg_dma_len() over binding->sgt to confirm it equals dmabuf->size.  If an
exporter's mapped sgt describes more bytes than dmabuf->size, does this
store net_iov contents, DMA addresses and net_iov pointers beyond the end
of the three allocations?

In the other direction, if the sgt describes fewer bytes than
dmabuf->size, the tail of tx_vec[] stays uninitialized (kvmalloc, not
zeroed), while net_devmem_get_niov_at() bounds the user-supplied offset
only against the dmabuf size:

	if (virt_addr >= binding->dmabuf->size)
		return NULL;
	...
	return binding->tx_vec[virt_addr / PAGE_SIZE];

Can that return a garbage pointer that is then used as a struct net_iov?
For the RX case the same shortfall would leave free_count below
area.num_niovs, so the teardown WARN in
__net_devmem_dmabuf_binding_free() quoted above would fire on every
unbind.

Before this series the RX niovs were sized per scatter-gather entry
(owner->area.num_niovs = len >> niov_shift), so an sgt/size mismatch
could not overrun them; the count is now derived from dmabuf->size with
no bound check added.

The io_uring zcrx code this series says it mirrors performs both checks -
io_import_dmabuf() sums sg_dma_len() and rejects a mismatch:

	for_each_sgtable_dma_sg(mem->sgt, sg, i)
		total_size += sg_dma_len(sg);

	if (total_size != len) {
		ret = -EINVAL;
		goto err;
	}

and io_populate_area_dma() bounds its fill loop with
while (sg_len && niov_idx < area->nia.num_niovs).  Would it make sense to
do the same here, either by validating the sg total against dmabuf->size
or by capping niov_idx at binding->area.num_niovs?

I could not identify an in-tree exporter that provably returns an sgt
whose total differs from dmabuf->size, so this may not be reachable
today; the gap does still appear to be present at the end of the series
(0942efb741d9 does not touch net_devmem_bind_dmabuf()).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917181116.3319017-1-sdf%40fomichev.me