Re: [PATCH net-next v3 8/9] 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 TX net_iovs grow from PAGE_SIZE to as much as
  2MB.

--

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

This patch changes how TX net_iovs are sized. Instead of PAGE_SIZE, each
TX net_iov is now the largest power of two from PAGE_SIZE up to 2MB that
aligns both the dmabuf size and every DMA segment in the sg table.
netdev_nl_bind_tx_doit() passes niov_shift == 0 to ask for this, so that
BIG TCP can build larger frags.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index b607cf495980c..74de200f66be2 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -388,6 +401,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);
> +	}
> +	binding->niov_shift = niov_shift;
> +	niov_size = 1UL << niov_shift;
> +

[Severity: Medium]
netdev_nl_bind_tx_doit() now passes niov_shift == 0, so a hugepage-backed
dmabuf gets 2MB TX net_iovs. Can a devmem TX frag then end up larger than
dev->netmem_tx_max_frag_size?

The cap is only applied when a frag is first built:

net/core/datagram.c:zerocopy_fill_skb_from_devmem() {
    ...
		size = min_t(size_t, size, binding->tx_max_frag_size);
    ...
}

Later, tcp_mtu_probe()->tcp_clone_payload() merges contiguous frags and
never checks the binding's limit:

net/ipv4/tcp_output.c:tcp_clone_payload() {
    ...
			if (lastfrag &&
			    skb_frag_page(fragfrom) == skb_frag_page(lastfrag) &&
			    skb_frag_off(fragfrom) == skb_frag_off(lastfrag) +
						      skb_frag_size(lastfrag)) {
				skb_frag_size_add(lastfrag, todo);
				continue;
			}
    ...
}

Before this patch, contiguous frags inside one net_iov could add up to at
most PAGE_SIZE. That is also the default netmem_tx_max_frag_size, so a
merged frag still stayed within the cap.

With 2MB net_iovs, contiguous TCP segments in the same net_iov can be
merged into one frag of up to probe_size bytes. On a path whose MTU is
above PAGE_SIZE, that frag is larger than the limit the device declared.

Documentation/networking/netmem.rst says:

    3. Devmem TX frags are capped at netdev->netmem_tx_max_frag_size,
       PAGE_SIZE by default.

The commit message says:

    frags are capped to the device limit when they are built

Is a cap applied only at build time enough to keep that contract?

The other merge paths are tcp_grow_skb() and SACK shifting. Both go
through skb_shift(), which bails out on skb_zcopy(), so tcp_clone_payload()
looks like the only path left.

All of these are needed to reach it:

  - an unprivileged bind-tx (NETDEV_CMD_BIND_TX has no GENL_ADMIN_PERM)
  - MTU probing turned on (tcp_mtu_probing=2, or 1 after black hole
    detection)
  - a path MTU above the device's frag limit

I could not find an in-tree driver that breaks today:

  - gve keeps the PAGE_SIZE default but splits buffers at
    GVE_TX_MAX_BUF_SIZE_DQO
  - bnxt and fbnic use 32K
  - mlx5 uses U32_MAX

A driver that relies on the documented cap would still get a frag larger
than it declared.

There is also a related issue in the same code. It is pre-existing and
was not introduced by this patch.

skb_frag_page() returns NULL for every net_iov frag. So for devmem frags
the skb_frag_page() comparison above is always true, even when the frags
come from different net_iovs.

Suppose the next frag's offset inside its net_iov equals the end offset
of lastfrag inside its own net_iov. Then lastfrag is extended over bytes
from the wrong net_iov, and the wrong dmabuf data is sent under real
sequence numbers. The extended range stays inside the first net_iov, so
there is no DMA outside the dmabuf.

Would either of these fix both problems:

  - compare skb_frag_netmem() and apply binding->tx_max_frag_size in
    tcp_clone_payload()
  - don't coalesce unreadable skbs when building MTU probes

[ ... ]

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