Re: [net-next v24 5/7] net: mtip: Add mtip_switch_{rx|tx} functions to the L2 switch driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Flawed error handling for multi-descriptor packets causes fragmented packet injection.
- [High] NULL pointer dereference in mtip_switch_tx() when queues are stopped and ring is empty.
- [High] Missing dma_sync_single_for_device() on RX error path causes data corruption for subsequent packets.
- [High] `swap_buffer()` corrupts the tail of packets whose length is not a multiple of 4 bytes.
- [High] Missing dma_rmb() after reading RX descriptor ownership allows speculative execution to read stale data.
- [High] The TX completion path unmaps DMA buffers using a hardcoded frame size instead of the original mapped length.
- [Medium] The Ethernet FCS is incorrectly included in the SKB payload, breaking bridging applications.
- [Low] Unreachable code: The TX completion check for `BD_ENET_TX_READY` is mathematically impossible.
- [Low] Received SKBs are built with zero headroom, leading to inefficient processing.
--

commit be1b7f747ef200958ab836f16e9cf7ce2caf1583
Author: Lukasz Majewski <lukasz.majewski@mailbox.org>

net: mtip: Add mtip_switch_{rx|tx} functions to the L2 switch driver
    
This patch provides mtip_switch_tx and mtip_switch_rx functions
code for MTIP L2 switch.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> index 7dfeda230e3fb..212207861e13e 100644
> --- a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
[ ... ]
> @@ -836,11 +873,233 @@ static irqreturn_t mtip_interrupt(int irq, void *ptr_fep)
>  
>  static void mtip_switch_tx(struct switch_enet_private *fep)
>  {
> +	struct net_device *dev;
> +	unsigned short status;
> +	struct sk_buff *skb;
> +	struct cbd_t *bdp;
> +	int index;
> +
> +	spin_lock_bh(&fep->hw_lock);
> +	bdp = fep->dirty_tx;
> +
> +	while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
> +		if (bdp == fep->cur_tx &&
> +		    !mtip_netif_queues_stopped(fep))
> +			break;

[Severity: High]
What happens if the queues are stopped and the ring is empty?

If the ring is empty (bdp == fep->cur_tx) but the queues are stopped
(e.g. link down), this break condition evaluates to false. Would this
cause the loop to process an empty, unsubmitted descriptor, leading to a
NULL pointer dereference when accessing fep->tx_skbuff[index] and
skb->dev below?

> +
> +		index = bdp - fep->tx_bd_base;
> +		dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
> +				 MTIP_SWITCH_TX_FRSIZE, DMA_TO_DEVICE);

[Severity: High]
Does this unmap call violate the DMA API by using a hardcoded length?

The DMA API mandates that the unmap size must exactly match the length
passed to dma_map_single(). Since outgoing network packets have variable
lengths, could using the hardcoded MTIP_SWITCH_TX_FRSIZE maximum size
cause DMA API warnings, IOMMU faults, or memory corruption?

> +		bdp->cbd_bufaddr = 0;
> +		skb = fep->tx_skbuff[index];
> +		dev = skb->dev;
> +		/* Check for errors */
> +		if (status & (BD_ENET_TX_HB | BD_ENET_TX_LC |
> +				   BD_ENET_TX_RL | BD_ENET_TX_UN |
> +				   BD_ENET_TX_CSL)) {
[ ... ]
> +		} else {
> +			dev->stats.tx_packets++;
> +		}
> +
> +		if (status & BD_ENET_TX_READY)
> +			dev_err_ratelimited(&fep->pdev->dev,
> +					    "xmit interrupt and TX_READY.\n");

[Severity: Low]
Is it mathematically possible to reach this condition?

The while loop entry condition guarantees that BD_ENET_TX_READY is zero:
while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0)

Could this check be unreachable dead code, indicating a potential logic
flaw in the state handling?

[ ... ]
> +static int mtip_switch_rx(struct net_device *dev, int budget)
>  {
> -	return -ENOMEM;
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	u8 *data, rx_port = MTIP_PORT_FORWARDING_INIT;
> +	struct switch_enet_private *fep = priv->fep;
> +	unsigned short status, pkt_len;
> +	struct net_device *pndev;
> +	struct ethhdr *eth_hdr;
> +	int pkt_received = 0;
> +	struct sk_buff *skb;
> +	struct cbd_t *bdp;
> +	struct page *page;
> +	int index;
> +
> +	/* First, grab all of the stats for the incoming packet.
> +	 * These get messed up if we get called due to a busy condition.
> +	 */
> +	bdp = fep->cur_rx;
> +
> +	while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) {

[Severity: High]
Is a read memory barrier needed here?

The loop reads the descriptor status to verify that the hardware has
yielded ownership. Without a dma_rmb() executed after this check, could
the CPU speculatively reorder subsequent reads of bdp->cbd_datlen and
the packet payload to occur before the ownership status change is
globally visible, leading to stale data being processed?

> +		if (pkt_received >= budget)
> +			break;
> +
> +		pkt_received++;
> +
> +		writel(MCF_ESW_IMR_RXF, fep->hwp + ESW_ISR);
> +		if (!fep->usage_count)
> +			goto rx_processing_done;
> +
> +		status ^= BD_ENET_RX_LAST;
> +		/* Check for errors. */
> +		if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO |
> +			      BD_ENET_RX_CR | BD_ENET_RX_OV | BD_ENET_RX_LAST |
> +			      BD_ENET_RX_CL)) {

[Severity: Critical]
Does this logic safely handle multi-descriptor packets?

The XOR logic (status ^= BD_ENET_RX_LAST) treats any descriptor without
the last-fragment bit as an error. The error handling path then yields the
descriptor back to the hardware but does not drain the remaining descriptors
of the packet.

When the final descriptor of the fragmented packet is received, it will
pass the error check, and the driver might incorrectly parse the middle or
end of the fragmented packet as a brand new Ethernet frame.

[ ... ]
> +		/* Get correct RX page */
> +		index = bdp - fep->rx_bd_base;
> +		page = fep->page[index];
> +		/* Process the incoming frame */
> +		pkt_len = bdp->cbd_datlen;
> +
> +		dma_sync_single_for_cpu(&fep->pdev->dev, bdp->cbd_bufaddr,
> +					pkt_len, DMA_FROM_DEVICE);
> +		net_prefetch(page_address(page));
> +		data = page_address(page);
> +
> +		if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
> +			swap_buffer(data, pkt_len);

[Severity: High]
Does swap_buffer() safely handle packet lengths that are not a multiple of 4?

Looking at the implementation:

static void swap_buffer(void *bufaddr, int len)
{
        unsigned int *buf = bufaddr;
        int i;

        for (i = 0; i < len; i += 4, buf++)
                swab32s(buf);
}

It appears the loop condition iterates strictly by 4. If the packet length
is not an exact multiple of 4 bytes, could the final iteration overshoot
the valid packet data, pulling uninitialized garbage bytes into the tail
of the packet?

> +
> +		eth_hdr = (struct ethhdr *)data;
> +		mtip_atable_get_entry_port_number(fep, eth_hdr->h_source,
> +						  &rx_port);
[ ... ]
> +		if (mtip_update_cbd(fep, bdp, index)) {
> +			pndev->stats.rx_dropped++;
> +			goto rx_processing_done;
> +		}

[Severity: High]
If this allocation fails, does the driver return a dirty buffer back to the
hardware?

When swap_buffer() modifies the buffer in-place, the CPU cache is dirtied.
If we take this error path, the buffer is yielded back to the hardware
(via rx_processing_done) without calling dma_sync_single_for_device() to
transfer ownership. Could the dirty CPU cache lines later be asynchronously
evicted to RAM, overwriting new packet data being DMA'd by the hardware?

> +
> +		/* The packet length includes FCS, but we don't want to
> +		 * include that when passing upstream as it messes up
> +		 * bridging applications.
> +		 */
> +		skb = build_skb(page_address(page), PAGE_SIZE);

[Severity: Low]
Does this allocation leave zero headroom for the SKB?

Incoming packets are DMA'd to offset 0 of the allocated page. By building
the SKB directly at page_address(page), the SKB is created with
skb->head == skb->data. While the networking core will safely reallocate
headers if necessary, failing to reserve standard headroom (like NET_SKB_PAD)
can cause unnecessary performance degradation.

> +		if (unlikely(!skb)) {
> +			page_pool_recycle_direct(fep->page_pool, page);
> +			pndev->stats.rx_dropped++;
> +
> +			netdev_err_once(pndev, "build_skb failed!\n");
> +			goto rx_processing_done;
> +		}
> +
> +		skb_put(skb, pkt_len);      /* Make room */

[Severity: Medium]
Does this skb_put() call incorrectly include the FCS in the SKB payload?

The comment immediately above states the intention to exclude the FCS
because it messes up bridging applications. However, this calls
skb_put() using the full pkt_len without subtracting ETH_FCS_LEN (4 bytes).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601112437.2216043-1-lukasz.majewski@mailbox.org?part=5