Re: [PATCH net-next v6 10/14] net: lan966x: add PCIe FDMA support

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 3

- [Medium] DISCARDED — fixed later in the series.
- [Low] DISCARDED — fixed later in the series.
- [Low] DISCARDED — fixed later in the series.
- [Low] DISCARDED — fixed later in the series.

Pre-existing issues:
- [High] Use-after-free in the new PCIe receive path:
  `lan966x_fdma_pci_rx_get_frame()`…
- [Medium] DISCARDED — pre-existing shared-library ordering weakness.

--

net: lan966x: add PCIe FDMA support

Adds a second FDMA backend for lan966x used when the switch is reached over
PCIe, using contiguous ATU-mapped DMA buffers and memcpy-based frame
transfer instead of per-page mappings. XDP and PTP are not advertised or
allowed on this path.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> new file mode 100644
> index 000000000000..f1f3c789d3a6
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> @@ -0,0 +1,404 @@

[ ... ]

> +/* Return true if blockl is a valid RX frame size. */
> +static bool lan966x_fdma_pci_rx_size_fits(struct fdma *fdma, u32 blockl)
> +{
> +	return blockl >= IFH_LEN_BYTES + ETH_HLEN + ETH_FCS_LEN &&
> +	       blockl <= fdma->db_size - XDP_PACKET_HEADROOM;
> +}

[ ... ]

> +	skb->protocol = eth_type_trans(skb, skb->dev);
> +
> +	if (lan966x->bridge_mask & BIT(src_port)) {
> +		skb->offload_fwd_mark = 1;
> +
> +		skb_reset_network_header(skb);
> +		if (!lan966x_hw_offload(lan966x, src_port, skb))
> +			skb->offload_fwd_mark = 0;
> +	}
> +
> +	skb->dev->stats.rx_bytes += skb->len;
> +	skb->dev->stats.rx_packets++;
> +
> +	return skb;
> +}

[Severity: High]
This isn't a bug introduced by this patch - lan966x_fdma_rx_get_frame() in
lan966x_fdma.c has the same caller pattern - but can lan966x_fdma_pci_rx_get_frame()
touch skb after lan966x_hw_offload() has already freed it?

lan966x_hw_offload() replaces the buffer and reports only a boolean:

drivers/net/ethernet/microchip/lan966x/lan966x_main.c:lan966x_hw_offload() {
	...
	if (eth_type_vlan(skb->protocol)) {
		skb = skb_vlan_untag(skb);
		if (unlikely(!skb))
			return false;
	}
	...
}

and skb_vlan_untag() frees the caller's skb on failure:

net/core/skbuff.c:skb_vlan_untag() {
	...
	if (unlikely(!pskb_may_pull(skb, VLAN_HLEN + sizeof(unsigned short))))
		goto err_free;
	...
err_free:
	kfree_skb(skb);
	return NULL;
}

So on the false return the caller's pointer is dangling, and
lan966x_fdma_pci_rx_get_frame() then does:

	skb->offload_fwd_mark = 0;
	skb->dev->stats.rx_bytes += skb->len;
	skb->dev->stats.rx_packets++;
	return skb;

with the returned skb going to napi_gro_receive() in
lan966x_fdma_pci_napi_poll().

Is the new lower bound in lan966x_fdma_pci_rx_size_fits() wide enough to let
this happen? IFH_LEN_BYTES + ETH_HLEN + ETH_FCS_LEN is 46, so a BLOCKL of
46..51 is accepted, and after skb_pull(IFH_LEN_BYTES),
skb_trim(skb->len - ETH_FCS_LEN) and eth_type_trans() there are only 0..5
bytes left. For a frame with h_proto 0x8100 on a bridged port that has IGMP
or MLD redirection enabled, the pskb_may_pull(VLAN_HLEN + 2) inside
skb_vlan_untag() then fails and the buffer is freed.

Would it make sense to have lan966x_hw_offload() take a struct sk_buff **,
or to have the caller stop using skb once it returns false?

>  static int lan966x_fdma_pci_xmit(struct sk_buff *skb, __be32 *ifh,

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671%40microchip.com