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

[sashiko-bot@kernel.org]

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

New issues:
- [High] NAPI polling loop reads incomplete CAN frames ahead of the DMA engine, corrupting the ring buffer read pointer.
- [High] DMA initialization failure breaks PIO fallback (hanging RX) and causes double-free on driver removal.

Pre-existing issues:
- [High] Missing bounds check on CAN frame data length allows a heap buffer overflow.
- [High] SKB allocation failure causes permanent NAPI deadlock and RX stream desynchronization.
--

commit 60c5f9dce521c51839cd00b85f3676b903c1d307
Author: Binbin Zhou <zhoubinbin@loongson.cn>

    can: loongson_canfd: Add RXDMA support

    Add optional DMA support for RX path using the Loongson APB CMC DMA
    engine. When a DMA channel is successfully requested, the driver:
    - Uses DMA cyclic transfers to write incoming CAN frames directly to
      a coherent DMA buffer

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> index 1c7381129c2b6..a4b5d9e506765 100644
> --- a/drivers/net/can/loongson_canfd.c
> +++ b/drivers/net/can/loongson_canfd.c
[ ... ]
> +static void loongson_canfd_rxdma_remove(struct loongson_canfd_priv *priv, struct device *dev)
> +{
> +	if (!priv->rx_ch)
> +		return;
> +
> +	dmaengine_terminate_sync(priv->rx_ch);
> +	loongson_canfd_rxdma_free(priv, dev);
> +	dma_release_channel(priv->rx_ch);
> +}
[ ... ]
> +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_ch->cookie, &state);
> +	if (status != DMA_IN_PROGRESS)
> +		return true;
> +
> +	return priv->last_res == (state.residue / DMA_SLAVE_BUSWIDTH_4_BYTES);
> +}

[Severity: High]
Could this implementation cause the CPU to read incomplete frames ahead of the 
DMA engine?

The cyclic DMA engine writes to memory word-by-word. As soon as the DMA 
transfers a single word (like meta0), the residue changes and this function 
returns false.

Because the NAPI loop executing on the CPU is faster than the DMA engine, it 
appears loongson_canfd_rx() will unconditionally read the entire frame 
(including meta1 and data words) before the DMA engine has actually transferred 
them. 

Does this structurally guarantee stream desynchronization and data corruption 
under load?

[ ... ]
> @@ -999,10 +1143,10 @@ static void loongson_canfd_read_rx_frame(struct loongson_canfd_priv *priv, struc
>  
>  	/* Data */
>  	for (i = 0; i < len; i += LOONGSON_CANFD_DW_BYTE)
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, (u32 *)(cf->data + i));
> +		*(u32 *)(cf->data + i) = priv->get_rx_data(priv);
>  
>  	while (unlikely(i < wc * LOONGSON_CANFD_DW_BYTE)) {
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
> +		priv->get_rx_data(priv);
>  		i += LOONGSON_CANFD_DW_BYTE;
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but is it possible for `len` to exceed the size 
of `cf->data` and cause a heap buffer overflow?

The `cf->data` buffer is a fixed 64-byte array. `len` is calculated based on 
the RWCNT field from meta1, but there appears to be no bounds check verifying 
that `len <= CANFD_MAX_DLEN`. 

If the hardware provides a malformed RWCNT, or if the CPU reads garbage data 
as meta1 (due to the DMA race mentioned above), won't this write past the end 
of the allocated SKB and corrupt the `skb_shared_info` and adjacent heap memory?

[ ... ]
> @@ -1291,12 +1436,11 @@ 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 what happens if SKB allocation 
fails inside loongson_canfd_rx()?

Looking at loongson_canfd_rx(), if alloc_can_skb() or alloc_canfd_skb() fails 
due to memory exhaustion, it returns 0 immediately without consuming the rest 
of the frame's words:

	if (unlikely(!skb))
		return 0;

This seems to cause two separate issues:
1. It desynchronizes the data stream, as subsequent polls will misinterpret 
   remaining payload words as the headers of the next frame.
2. It breaks the NAPI loop because `ret == 0` causes this completion condition 
   (`rxbuf_is_empty && ret != 0`) to evaluate to false. 

By skipping napi_complete_done() and returning a value less than the quota, the 
NAPI state bit NAPIF_STATE_SCHED remains permanently set but removed from the 
poll list. Doesn't this permanently deadlock the receiver until a module reload?

[ ... ]
> @@ -1646,6 +1791,24 @@ static int loongson_canfd_probe(struct platform_device *pdev)
>  	ndev->ethtool_ops = &loongson_canfd_ethtool_ops;
>  	SET_NETDEV_DEV(ndev, dev);
>  
> +	priv->get_rx_data = loongson_canfd_get_rxdata_in_poll;
> +	priv->get_rxbuf_empty = loongson_canfd_get_rxbuf_empty_in_poll;
> +
> +	priv->rx_ch = dma_request_chan(dev, "rx");
> +	if (PTR_ERR(priv->rx_ch) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (IS_ERR(priv->rx_ch)) {
> +		dev_warn(dev, "Fall back in poll mode for any non-deferral error.\n");
> +		priv->rx_ch = NULL;
> +	}
> +
> +	ret = loongson_canfd_rxdma_init(ndev);
> +	if (!ret) {
> +		priv->get_rx_data = loongson_canfd_get_rxdata_in_dma;
> +		priv->get_rxbuf_empty = loongson_canfd_get_rxbuf_empty_in_dma;
> +		priv->last_res = LOONGSON_CANFD_RXDMA_NUM;
> +	}
>  	netif_napi_add(ndev, &priv->napi, loongson_canfd_rx_napi);

[Severity: High]
What happens if loongson_canfd_rxdma_init() fails?

If loongson_canfd_rxdma_init() returns an error (e.g., dma_alloc_coherent fails), 
it frees the buffer but does not clear priv->rx_ch or release the channel. The 
probe function ignores the error and proceeds with the intent to fall back to PIO.

However, since priv->rx_ch remains non-NULL, loongson_canfd_chip_start() will 
incorrectly configure the controller to use the DMADI interrupt instead of the 
RBNEI PIO interrupt, permanently breaking reception.

Additionally, upon module removal, loongson_canfd_rxdma_remove() checks 
priv->rx_ch and calls loongson_canfd_rxdma_free() again. Since the buffer was 
already freed during the failed init, won't this trigger a double-free?

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