Re: [PATCH net-next v4 4/5] net: devmem: size TX net_iovs from the dmabuf sg table

[netdev-bot+sashiko@kernel.org]

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] This patch makes devmem TX net_iovs up to 2MB.

--

net: devmem: size TX net_iovs from the dmabuf sg table

This commit lets TX devmem bindings choose the net_iov size from the
dmabuf sg table. It uses the largest power of two between PAGE_SIZE and
2MB that aligns every DMA segment. netdev_nl_bind_tx_doit() now passes
niov_shift 0 to request this, so hugepage-backed udmabufs get 2MB TX
net_iovs.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index c6868f39261ad..7135280488971 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -387,6 +400,17 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  		goto err_unmap;
>  	}
>  
> +	if (!niov_shift) {
> +		niov_size = SZ_2M;
> +		while (niov_size > PAGE_SIZE &&
> +		       (!IS_ALIGNED(dmabuf->size, niov_size) ||
> +			!net_devmem_sgt_aligned(binding->sgt, niov_size)))
> +			niov_size /= 2;
> +		niov_shift = ilog2(niov_size);
> +	}

[Severity: Medium]
netdev_nl_bind_tx_doit() now passes 0 here. That means a 2MB aligned,
hugepage-backed dmabuf gets 2MB TX net_iovs no matter which device it is
bound to. Can this produce frags that bnxt silently truncates?

Before this patch, every devmem TX frag was capped at PAGE_SIZE. Now
zerocopy_fill_skb_from_devmem() limits a frag only by niov_size - off,
the copy length and the iov length:

net/core/datagram.c:zerocopy_fill_skb_from_devmem() {
    ...
		size = min_t(size_t, size, length);
		size = min_t(size_t, size, iter_iov_len(from));

		netmem_inc_ref(net_iov_to_netmem(niov));
		skb_add_rx_frag_netmem(skb, i, net_iov_to_netmem(niov), off,
				       size, size);
    ...
}

So with BIG TCP enabled (gso_max_size > 65536), a single frag can be 64KB
or larger.

In bnxt_init_one(), bnxt sets dev->netmem_tx = NETMEM_TX_DMA and calls
netif_set_tso_max_size(dev, GSO_MAX_SIZE). bnxt_start_xmit() then writes
the frag length into the BD without checking its size:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_start_xmit() {
    ...
		len = skb_frag_size(frag);
		mapping = skb_frag_dma_map(&pdev->dev, frag, 0, len,
					   DMA_TO_DEVICE);
    ...
		flags = len << TX_BD_LEN_SHIFT;
		txbd->tx_bd_len_flags_type = cpu_to_le32(flags);
    ...
}

TX_BD_LEN is (0xffff << 16), so the length field is 16 bits wide. A 65536
byte frag would be written as 0, and a 70000 byte frag as 4464. The DMA
mapping and BQL still use the full length. Depending on the hardware, the
result could be a corrupted LSO payload on the wire or a TX error and
ring reset.

The "Dropped oversize" check in bnxt_start_xmit() only looks at gso_size
plus hdr_len for GSO skbs, so it does not catch this.

The commit message says:

    bnxt doesn't handle them yet. It has a 16-bit BD length and needs a
    separate fix.

That fix does not appear in this series, and it is not listed as a
prerequisite. The only bnxt change in the series is "bnxt_en: let the
stack segment GSO skbs with too many frags". It handles nr_frags >
TX_MAX_FRAGS in bnxt_features_check(), but it does not limit frag size.

NETDEV_CMD_BIND_TX does not require GENL_ADMIN_PERM. Once an admin enables
BIG TCP on a bnxt port, any local user who can create such a dmabuf (for
example a udmabuf from a hugetlb memfd) could reach this path.

MSG_ZEROCOPY from huge pages can already hit the same truncation. This
patch adds a new path to it through devmem TX, in the BIG TCP setup the
patch is meant to enable.

Should the auto-selected niov size be capped per device until bnxt is
fixed? Or could the bnxt fix land before this patch?

> +	binding->niov_shift = niov_shift;
> +	niov_size = 1UL << niov_shift;
> +
>  	if (!IS_ALIGNED(dmabuf->size, niov_size)) {
>  		err = -EINVAL;
>  		NL_SET_ERR_MSG_FMT(extack,

[ ... ]

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