Re: [PATCH net-next 2/3] net: devmem: use memory provider helpers for net_iovs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] net: devmem: use memory provider helpers for net_iovs bundles a
  real bug fix into a patch whose commit message presents it as a…

--

net: devmem: use memory provider helpers for net_iovs

This converts net/core/devmem.c to the public memory-provider helpers
net_mp_niov_set_dma_addr(), net_mp_niov_set_page_pool() and
net_mp_niov_clear_page_pool() instead of open-coding page_pool state,
switches the net_iov array to kvzalloc_objs(), and now rejects DMA
addresses that cannot be represented in net_iov metadata.

>  	Zero the net_iov array because net_mp_niov_set_page_pool() ORs
>  	PP_SIGNATURE into pp_magic. Reject DMA addresses that cannot be
>  	represented in net_iov metadata.

[Severity: Low]
This isn't a bug in the new code, but should the "Reject DMA addresses
that cannot be represented in net_iov metadata" sentence spell out that
the old behaviour was a silent truncation, and should the patch carry a
Fixes: tag?

Before this patch net_devmem_bind_dmabuf() dropped the return value of
page_pool_set_dma_addr_netmem(), which is the "this address does not
round-trip through the compressed net_iov metadata" indicator:

net/core/page_pool_priv.h:page_pool_set_dma_addr_netmem() {
	if (PAGE_POOL_32BIT_ARCH_WITH_64BIT_DMA) {
		netmem_set_dma_addr(netmem, addr >> PAGE_SHIFT);

		/* We assume page alignment to shave off bottom bits,
		 * if this "compression" doesn't work we need to drop.
		 */
		return addr != (dma_addr_t)netmem_get_dma_addr(netmem)
				       << PAGE_SHIFT;
	}
	...
}

Every other caller treats a true return as fatal, e.g.
page_pool_dma_map():

	if (page_pool_set_dma_addr_netmem(netmem, dma)) {
		WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
		goto unmap_failed;
	}

and io_uring/zcrx.c returns -EFAULT. Devmem was the only site that kept
the truncated address and handed it to the NIC for DMA.

The path looks reachable from user space on 32-bit arches with 64-bit
dma_addr_t: netdev_nl_bind_rx_doit() only checks is_power_of_2() on
NETDEV_A_DMABUF_RX_PAGE_SIZE before niov_shift = ilog2(rx_page_size), so
a sub-PAGE_SIZE niov size is accepted, and the bind loop then advances
dma_addr += niov_size producing non-page-aligned addresses whose
compression fails.

Given that the next patch in the series ("net: devmem: decode DMA
addresses for TX") does carry a Fixes: tag for the other half of the same
32-bit/64-bit-DMA problem, would it make sense to split this hunk out
with its own Fixes: tag so it can be picked up for stable?

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index c1c1872b88de2..0fc77c1a29569 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -274,8 +272,12 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  		for (i = 0; i < nr_niovs; i++, niov_idx++) {
>  			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 (net_mp_niov_set_dma_addr(niov, dma_addr)) {
> +				err = -EFAULT;
> +				NL_SET_ERR_MSG(extack,
> +					       "dmabuf DMA address cannot be represented");
> +				goto err_free_niovs;
> +			}
>  			if (direction == DMA_TO_DEVICE)
>  				binding->tx_vec[niov_idx] = niov;
>  			else

[ ... ]

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