Re: [PATCH net-next] net: octeon: Fix dma_unmap_single() size

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (2) · 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).

- [High] The patch does not fix the size mismatch it claims to fix.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  octeon_mgmt_stop()…
- [Medium] This problem wasn't introduced by this patch, but the RX
  descriptor `len` field is only 14 bits (max 16383), while the MTU can…

--

net: octeon: Fix dma_unmap_single() size

The commit changes the RX buffer unmap size in
octeon_mgmt_dequeue_rx_buffer() from ETH_FRAME_LEN +
OCTEON_MGMT_RX_HEADROOM to re.s.len. The goal is to match the size used
when the buffer was mapped in octeon_mgmt_rx_fill_ring().

> diff --git a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
> index 2cf3365b96364..1bb1316f4b919 100644
> --- a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
> +++ b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
> @@ -380,9 +380,7 @@ static u64 octeon_mgmt_dequeue_rx_buffer(struct octeon_mgmt *p,
>  	p->rx_current_fill--;
>  	*pskb = __skb_dequeue(&p->rx_list);
>  
> -	dma_unmap_single(p->dev, re.s.addr,
> -			 ETH_FRAME_LEN + OCTEON_MGMT_RX_HEADROOM,
> -			 DMA_FROM_DEVICE);
> +	dma_unmap_single(p->dev, re.s.addr, re.s.len, DMA_FROM_DEVICE);

[Severity: High]
Does re.s.len still hold the mapped size here?

re.d64 is read from p->rx_ring[] after the MIX engine has written back
the completed descriptor. The caller treats len as the number of bytes
received:

octeon_mgmt_receive_one() {
    re.d64 = octeon_mgmt_dequeue_rx_buffer(p, &skb);
    if (likely(re.s.code == RING_ENTRY_CODE_DONE)) {
        /* A good packet, send it up. */
        skb_put(skb, re.s.len);
    ...
}

The RING_ENTRY_CODE_MORE path does the same with re2.s.len. The code
field also changes from 0 (written by the driver) to DONE, MORE or an
error value. That suggests the hardware rewrote the whole 64-bit entry.

The buffer was mapped in octeon_mgmt_rx_fill_ring() with:

    size = netdev->mtu + OCTEON_MGMT_RX_HEADROOM + 8 + NET_IP_ALIGN;
    ...
    re.s.len = size;
    re.s.addr = dma_map_single(p->dev, skb->data,
                               size,
                               DMA_FROM_DEVICE);

At MTU 1500 the mapping is 1532 bytes. For a minimum-size frame, this
would unmap only about 60 bytes of it. On error entries the size is
whatever the hardware reported.

With CONFIG_DMA_API_DEBUG, would check_unmap() report a size mismatch on
almost every received packet? And with swiotlb=force, would only len
bytes be bounced back?

The TX side's dma_unmap_single(..., re.s.len, DMA_TO_DEVICE) works
because TX entries keep the length the driver wrote. RX entries don't.

Recomputing the size from netdev->mtu at dequeue time wouldn't work
either. The RING_ENTRY_CODE_MORE comment says buffers sized for an older
MTU can stay in the ring after an MTU change.

The len field is also only 14 bits wide, so even the fill-time value is
truncated for MTUs of 16352 and above.

Could the mapped length be recorded per ring slot instead? For example,
octeon_mgmt_rx_fill_ring() could store it in a shadow array indexed by
ring position.

>  
>  	return re.d64;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but octeon_mgmt_stop() frees
the remaining RX and TX buffers without unmapping them:

drivers/net/ethernet/cavium/octeon/octeon_mgmt.c:octeon_mgmt_stop() {
    ...
	/* dma_unmap is a nop on Octeon, so just free everything.  */
	skb_queue_purge(&p->tx_list);
	skb_queue_purge(&p->rx_list);
    ...
}

Are the DMA_FROM_DEVICE mappings from octeon_mgmt_rx_fill_ring() and
the DMA_TO_DEVICE mappings from octeon_mgmt_xmit() leaked on every
ifdown? The RX ring is normally kept full, so posted buffers are always
there at that point.

With the coherent dma-direct setup on Octeon, unmap is close to a no-op.
However, CONFIG_DMA_API_DEBUG would report the leaked mappings, and
swiotlb=force would leak bounce slots over repeated ifup/ifdown cycles.

[Severity: Medium]
This is also a pre-existing issue, not introduced by this patch. Can
size overflow the 14-bit len field in octeon_mgmt_rx_fill_ring()?

octeon_mgmt_probe() sets:

	netdev->max_mtu = 16383 - OCTEON_MGMT_RX_HEADROOM - VLAN_HLEN;

which is 16383 - 22 - 4 = 16357.

octeon_mgmt_rx_fill_ring() then computes:

	size = netdev->mtu + OCTEON_MGMT_RX_HEADROOM + 8 + NET_IP_ALIGN;

which is mtu + 32, or 16389 at max_mtu.

For MTUs from 16352 to 16357, re.s.len = size wraps modulo 16384 (to 5
at max_mtu). dma_map_single() still maps the full size.

Wouldn't the hardware then be told the buffer is only a few bytes long?
Large frames could be split across many MORE entries or dropped as
errors. The exact effect depends on how the MIX hardware uses the
length it is given.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015131336.34801-1-fourier.thomas%40gmail.com