Re: [PATCH v5 2/2] can: loongson_canfd: Add RXDMA support

[sashiko-bot@kernel.org]

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

New issues:
- [High] RX DMA ring buffer synchronization is flawed, allowing the CPU to read ahead of the DMA engine, causing data corruption and permanent desynchronization.
- [High] An inverted DMA status check causes an infinite NAPI polling loop if the DMA channel encounters an error.
- [High] Direct MMIO reads during DMA mode in error handling paths steal data from the hardware FIFO and corrupt the DMA stream.
- [Medium] Improper error handling during DMA initialization causes a permanent DMA channel leak and leaves the driver in a broken hybrid state.

Pre-existing issues:
- [Critical] Lack of bounds checking on the frame length allows a malicious CAN frame or corrupted DMA state to overflow the SKB data buffer.
- [High] The NAPI poll routine causes a softirq infinite loop (CPU soft lockup) if an SKB allocation fails.
--

--- Patch [2]: [PATCH v5 2/2] can: loongson_canfd: Add RXDMA support ---
commit b57ca3985f14b046d9875c2283806844564b67d5
Author: Binbin Zhou <zhoubinbin@loongson.cn>

can: loongson_canfd: Add RXDMA support

This commit extends the driver with optional DMA engine support for the
receive path. It maps a cyclic DMA transfer to continuously fill a ring
buffer to reduce MMIO accesses. The driver selects the read method
automatically based on DMA availability.

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> index 0e85005a9bb12..bed64c15d22a6 100644
> --- a/drivers/net/can/loongson_canfd.c
> +++ b/drivers/net/can/loongson_canfd.c
> @@ -546,13 +566,141 @@ static void loongson_canfd_set_txbuf_cmd(struct net_device *ndev,
[ ... ]
> +static bool loongson_canfd_get_rxbuf_empty_in_dma(struct loongson_canfd_priv *priv)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +
> +	status = dmaengine_tx_status(priv->rx_ch, priv->rx_cookie, &state);
> +
> +	return priv->last_res == (state.residue / DMA_SLAVE_BUSWIDTH_4_BYTES) &&
> +	       (status == DMA_IN_PROGRESS);
> +}

[Severity: High]
Does the strict equality check against the DMA residue prevent partial frame
reads? If the DMA engine is actively writing a frame when this is called, it
appears rxbuf_is_empty will evaluate to false, leading loongson_canfd_rx() to
read an entire frame. This could cause the CPU to read unwritten data and
decrement priv->last_res past the DMA write pointer, permanently
desynchronizing the CPU from the DMA engine.

[Severity: High]
Can the status check cause an infinite NAPI polling loop? If the DMA engine
enters an error state, status == DMA_IN_PROGRESS evaluates to false, causing
this function to return false (not empty). The NAPI loop would then repeatedly
call loongson_canfd_rx() to process non-existent data, leading to a CPU soft
lockup.

[ ... ]
> +static int loongson_canfd_rxdma_init(struct net_device *ndev)
> +{
> +	struct loongson_canfd_priv *priv = netdev_priv(ndev);
> +	struct dma_async_tx_descriptor *desc = NULL;
> +	struct device *dev = ndev->dev.parent;
> +	struct dma_slave_config config;
> +	int ret;
> +
> +	if (!priv->rx_ch)
> +		return -EINVAL;
> +
> +	priv->rx_buf = dma_alloc_coherent(dev, LOONGSON_CANFD_RXBUF_SZ,
> +					  &priv->rx_dma_buf, GFP_KERNEL);
> +	if (!priv->rx_buf)
> +		return -ENOMEM;

[Severity: Medium]
If this allocation fails, the function returns -ENOMEM but leaves priv->rx_ch
populated. During probe, this failure allows the driver to fall back to poll
mode, but since priv->rx_ch is non-NULL, does this incorrectly configure the
CAN interrupts for DMA (REG_INT_STAT_DMADI) instead of RBNEI, breaking the
receive path?

> +
> +	/* Configure DMA channel */
> +	memset(&config, 0, sizeof(config));
> +	config.src_addr = priv->res->start + LOONGSON_CANFD_RX_DATA;
> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +
> +	ret = dmaengine_slave_config(priv->rx_ch, &config);
> +	if (ret < 0) {
> +		netdev_err(ndev, "Loongson canfd rxdma channel config failed\n");
> +		goto err_config;
> +	}
[ ... ]
> +err_config:
> +	loongson_canfd_rxdma_free(priv, dev);
> +	priv->rx_ch = NULL;
> +	return ret;
> +}

[Severity: Medium]
Does the err_config error path leak the DMA channel? It sets priv->rx_ch to
NULL without calling dma_release_channel() on it.

> @@ -980,8 +1131,8 @@ static int loongson_canfd_rx(struct net_device *ndev)
>  	struct sk_buff *skb;
>  	bool is_can_fd;
>  
> -	regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &meta0);
> -	regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &meta1);
> +	meta0 = priv->get_rx_data(priv);
> +	meta1 = priv->get_rx_data(priv);
>  
>  	/* Number of characters received */
>  	rwcnt = FIELD_GET(REG_FRAME_META1_RWCNT, meta1);
>  	if (!rwcnt)
>  		return -EAGAIN;
>  	dbcnt = (rwcnt - 2) * LOONGSON_CANFD_DW_BYTE;

[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for dbcnt to
exceed the SKB data buffer size? For classic CAN frames, alloc_can_skb()
allocates an 8-byte buffer. If rwcnt is large, such as from a corrupted DMA
stream or maliciously crafted frame, dbcnt could reach up to 116 bytes,
overflowing the SKB buffer.

[ ... ]
> @@ -1033,7 +1184,7 @@ static int loongson_canfd_rx(struct net_device *ndev)
>  
>  	/* Data */
>  	for (i = 0; i < dbcnt; i += LOONGSON_CANFD_DW_BYTE)
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, (u32 *)(cfd->data + i));
> +		*(u32 *)(cfd->data + i) = priv->get_rx_data(priv);
>  
>  	while (unlikely(i < dbcnt)) {
>  		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
>  		i += LOONGSON_CANFD_DW_BYTE;
>  	}

[Severity: High]
Does this residual drain loop break the DMA stream by hardcoding regmap_read()
directly from the MMIO FIFO instead of using the priv->get_rx_data() callback?
In DMA mode, reading directly from the hardware FIFO steals words from the DMA
engine, corrupting the CAN frames before they reach RAM and bypassing the DMA
ring buffer pointer.

This same issue also appears in the SKB allocation failure path:

loongson_canfd_rx() {
	...
	if (unlikely(!skb)) {
		for (i = 0; i < dbcnt; i += LOONGSON_CANFD_DW_BYTE)
			regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
		ndev->stats.rx_dropped++;
		return 0;
	}
	...
}

Does the error path need to be updated to use priv->get_rx_data() as well?

> @@ -1256,12 +1407,12 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
>  	bool rxbuf_is_empty;
>  	u32 sts;
>  
> -	rxbuf_is_empty = loongson_canfd_rxbuf_empty(priv);
> +	rxbuf_is_empty = priv->get_rxbuf_empty(priv);
>  
>  	while (!rxbuf_is_empty && work_done < quota && ret > 0) {
>  		ret = loongson_canfd_rx(ndev);
>  		work_done++;
> -		rxbuf_is_empty = loongson_canfd_rxbuf_empty(priv);
> +		rxbuf_is_empty = priv->get_rxbuf_empty(priv);
>  	}
>  
>  	/* Check for RX FIFO Overflow */
[ ... ]
> @@ -1288,13 +1439,18 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
>  
>  	if (rxbuf_is_empty && ret != 0) {
>  		if (napi_complete_done(napi, work_done)) {

[Severity: High]
This isn't a bug introduced by this patch, but can this cause an infinite loop
in softirq context? If an SKB allocation fails in loongson_canfd_rx(), it
returns 0. The while loop breaks, and since ret == 0, the check here skips
napi_complete_done(). The NAPI instance remains on the active poll list,
causing the network core to repeatedly re-invoke the poll function until memory
is available.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783670733.git.zhoubinbin@loongson.cn?part=2