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

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 09/23, netdev-bot+sashiko@kernel.org wrote:
> 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?

I do not think this suggestion makes any sense. We do need to check
the return value of page_pool_set_dma_addr_netmem. Whether it's this
patch (I think it fits better here), or next - does not matter much.