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

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.

quoted

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 @@

[ ... ]

quoted

+/* 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;
+}

[ ... ]

quoted

+     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?

Yes, but pre-existing. I think this deserves a proper fix through net and
updates for all callers.

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?

quoted

 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