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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing memory barrier between reading RX packet data and handing the descriptor back to the hardware.

Pre-existing issues:
- [Critical] Use-After-Free on `skb` due to pointer invalidation inside `lan966x_hw_offload`.
- [High] Missing SKB length validation before reading IP/IPv6 headers.
- [High] DCB initialization in `__fdma_dcb_add` writes the link pointer before initializing the new DCB, creating a race with the DMA engine.
--

commit 6962735ae0c2fc198779cdd8603a67a7aa3b15a3
Author: Daniel Machon <daniel.machon@microchip.com>
Subject: net: lan966x: add PCIe FDMA support

This patch adds PCIe FDMA support for the lan966x driver, mapping contiguous
DMA buffers through the endpoint's ATU. This approach uses memcpy-based frame
transfers instead of per-page DMA mappings, which significantly increases
throughput.

> 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 0000000000000..f1f3c789d3a6e
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
[ ... ]
> +static struct sk_buff *lan966x_fdma_pci_rx_get_frame(struct lan966x_rx *rx,
> +						     u64 src_port)
> +{
[ ... ]
> +	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;

[Severity: Critical]
This is a pre-existing issue, but can this sequence lead to a use-after-free
on the skb pointer?

If a malformed or truncated VLAN packet is received, lan966x_hw_offload()
calls skb_vlan_untag(skb). If skb_vlan_untag() fails to pull the VLAN header,
it frees the skb internally and returns NULL, prompting lan966x_hw_offload()
to return false. The caller in lan966x_fdma_pci_rx_get_frame() then proceeds
to access the freed skb pointer to clear skb->offload_fwd_mark, which could
cause a kernel panic.

[Severity: High]
This is also a pre-existing issue in lan966x_hw_offload(), but is there a
missing length validation before reading the IP or IPv6 headers?

If a packet arrives with an IP or IPv6 ethertype but has a truncated payload
that is shorter than the header size, lan966x_hw_offload() casts the network
header and dereferences fields like ip_hdr(skb)->protocol without verifying
if the skb contains enough linear data via pskb_may_pull(). This could result
in an out-of-bounds read of kernel memory.

> +	}
> +
> +	skb->dev->stats.rx_bytes += skb->len;
> +	skb->dev->stats.rx_packets++;
> +
> +	return skb;
> +}
[ ... ]
> +static int lan966x_fdma_pci_napi_poll(struct napi_struct *napi, int weight)
> +{
[ ... ]
> +	/* Get all received skbs. */
> +	while (counter < weight) {
> +		if (!fdma_has_frames(fdma))
> +			break;
> +		/* Order DONE read before DCB/frame reads below. */
> +		dma_rmb();
> +		counter++;
[ ... ]
> +	}
> +	while (dcb_reload != fdma->dcb_index) {
> +		old_dcb = dcb_reload;
> +		dcb_reload++;
> +		dcb_reload &= fdma->n_dcbs - 1;
> +
> +		fdma_dcb_add(fdma,
> +			     old_dcb,
> +			     FDMA_DCB_INFO_DATAL(fdma->db_size - XDP_PACKET_HEADROOM),
> +			     FDMA_DCB_STATUS_INTR);

[Severity: High]
Does this loop need a memory barrier before writing the descriptor status back
to the hardware?

The CPU reads the incoming packet payload from a contiguous coherent DMA buffer
via memcpy() during lan966x_fdma_pci_rx_get_frame(), and then iterates here to
reclaim used descriptors by writing to their status fields in the same coherent
memory space. Because there is no general memory barrier (like mb() or
smp_mb()) separating the payload read from this descriptor write, the
operations might be reordered. If the descriptor write commits to memory before
the payload read completes, the device could start DMAing a new packet into the
buffer while the CPU is still reading the old one, leading to silent data
corruption.

[Severity: High]
This is a pre-existing issue in the fdma api, but does the call to
fdma_dcb_add() create a race condition with the DMA engine?

When appending a new DMA Control Block (DCB), __fdma_dcb_add() updates the
hardware-visible nextptr of the current tail DCB to point to the new DCB before
the new DCB's own nextptr (to mark the list end) and info fields are properly
initialized:

drivers/net/ethernet/microchip/fdma/fdma_api.c:__fdma_dcb_add() {
    err = dcb_cb(fdma, dcb_idx, &fdma->last_dcb->nextptr);
    if (unlikely(err))
        return err;

    fdma->last_dcb = dcb;
    dcb->nextptr = FDMA_DCB_INVALID_DATA;
    dcb->info = info;
}

If the DMA engine is actively processing the list, could it follow the updated
nextptr and consume the new DCB while it still contains uninitialized or stale
data from a previous cycle?

> +
> +		lan966x_fdma_rx_reload(rx);
> +	}
> +
> +	if (counter < weight && napi_complete_done(napi, counter))
> +		lan_wr(0xff, lan966x, FDMA_INTR_DB_ENA);
> +
> +	return counter;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671@microchip.com?part=10