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

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

New issues:
- [Critical] NAPI reads ahead of the DMA write pointer, leading to stale data reads and permanent ring buffer desynchronization.
- [High] The DMA receive buffer is freed before the DMA engine is terminated, causing hardware writes to freed memory.
- [High] Clearing the DMADI interrupt status at the end of NAPI causes lost wakeups for newly arrived frames.

Pre-existing issues:
- [Critical] Missing bounds check and integer underflow on `rwcnt` leads to a massive kernel heap buffer overflow.
--

--- Patch [2]: [PATCH v6 2/2] can: loongson_canfd: Add RXDMA support ---
commit e9fb5c097416d3376c7ac83b960fa3ca5f984b78
Author: Binbin Zhou <[email protected]>

can: loongson_canfd: Add RXDMA support
    
Extend the driver with an optional DMA engine support for the receive
path. When a DMA channel named rx is available (e.g., from the
loongson2-apb-cmc-dma driver), the RX buffer is mapped to a cyclic DMA
transfer that continuously fills a pre-allocated ring buffer. The driver
then readds incoming CAN frames directly from the DMA buffer, reducing
the number of MMIO accesses and improving performance under high
traffic.

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> index 211133258b083..d32c834a11264 100644
> --- a/drivers/net/can/loongson_canfd.c
> +++ b/drivers/net/can/loongson_canfd.c

[ ... ]

> @@ -521,13 +541,150 @@ 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);
> +	if (status != DMA_IN_PROGRESS)
> +		return true;
> +
> +	return priv->last_res == (state.residue / DMA_SLAVE_BUSWIDTH_4_BYTES);

[Severity: Critical]
Will this residue check allow NAPI to read ahead of the DMA write pointer?

Since the residue changes as soon as a single word is written, the CPU will
detect data is ready before a complete frame has been transferred. When
loongson_canfd_rx() is called, it will decrement priv->last_res to
unconditionally read multiple words (meta0, meta1, and payload), which might
result in fetching unwritten or stale memory for length fields and payload.

> +}
> +
> +static void loongson_canfd_rxdma_free(struct loongson_canfd_priv *priv, struct device *dev)
> +{
> +	if (!priv->rx_buf)
> +		return;
> +
> +	dma_free_coherent(dev, LOONGSON_CANFD_RXBUF_SZ, priv->rx_buf, priv->rx_dma_buf);
> +	priv->rx_buf = NULL;
> +}
> +
> +static void loongson_canfd_rxdma_remove(struct loongson_canfd_priv *priv, struct device *dev)
> +{
> +	if (!priv->rx_ch)
> +		return;
> +
> +	loongson_canfd_rxdma_free(priv, dev);
> +	dmaengine_terminate_sync(priv->rx_ch);

[Severity: High]
Does this free the DMA buffer while the cyclic DMA channel is still active?

Calling loongson_canfd_rxdma_free() before dmaengine_terminate_sync() could
allow the DMA controller to continue writing to pages that have been freed
and potentially reallocated to other kernel structures, which might cause
memory corruption during module unload or if probe fails.

[ ... ]

> @@ -949,14 +1109,14 @@ static int loongson_canfd_rx(struct net_device *ndev)
>  {
>  	struct loongson_canfd_priv *priv = netdev_priv(ndev);
>  	struct net_device_stats *stats = &ndev->stats;
> -	u32 meta0, meta1, dlc, rwcnt, dbcnt, i, data;
> +	u32 meta0, meta1, dlc, rwcnt, dbcnt, i;
>  	struct canfd_frame *cfd;
>  	struct can_frame *ccf;
>  	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 is a pre-existing issue, but does this calculation need bounds checking
or underflow protection?

If rwcnt is parsed from stale memory as 1 (which might happen due to the DMA
read-ahead behavior mentioned above), rwcnt - 2 will underflow, resulting in
a very large dbcnt.

[ ... ]

> @@ -1008,11 +1168,11 @@ static int loongson_canfd_rx(struct net_device *ndev)
>  
>  	/* Copy payload */
>  	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);

[Severity: Critical]
This is also a pre-existing issue, but if dbcnt is huge due to the integer
underflow mentioned above, will this loop cause a heap buffer overflow by
copying gigabytes of data into cfd->data?

[ ... ]

> @@ -1259,13 +1419,18 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
>  
>  	if (rxbuf_is_empty) {
>  		if (napi_complete_done(napi, work_done)) {
> +			int int_ena;
>  			/*
> -			 * Clear and enable RBNEI. It is level-triggered,
> +			 * Clear and enable RBNEI/DMADI. It is level-triggered, so
>  			 * so there is no race condition.
>  			 */
> -			regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, REG_INT_STAT_RBNEI);
> -			regmap_write(priv->regmap, LOONGSON_CANFD_INT_MASK,
> -				     (REG_INT_STAT_RBNEI << 16));
> +			if (priv->rx_ch)
> +				int_ena = REG_INT_STAT_DMADI;
> +			else
> +				int_ena = REG_INT_STAT_RBNEI;
> +
> +			regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, int_ena);
> +			regmap_write(priv->regmap, LOONGSON_CANFD_INT_MASK, (int_ena << 16));

[Severity: High]
Does unconditionally clearing the DMADI interrupt flag here cause a lost
wakeup?

If a frame finishes its DMA transfer after the rxbuf_is_empty check but
before this write to LOONGSON_CANFD_INT_STAT, the hardware sets the DMADI
flag, which will then be cleared immediately. Because it is cleared before
unmasking, the interrupt for the new frame could be lost, potentially
stalling incoming CAN frames.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2