Re: [PATCH v7 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] NAPI RX polling races with DMA engine word-by-word transfers, causing frame corruption and permanent stream desynchronization.
- [High] Spurious reads and desynchronization due to logic flaw when DMA residue reaches zero.
- [Medium] Silent data loss and misalignment due to lack of RX DMA cyclic buffer overflow detection.
- [Medium] Missing dma_rmb() causes reads of stale coherent DMA memory.

Pre-existing issues:
- [Critical] Heap buffer overflow when receiving Classical CAN frames due to missing length validation.
- [High] Incomplete consumption of invalid frames leads to permanent RX stream desynchronization.
--

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

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, the RX buffer is
mapped to a cyclic DMA transfer that continuously fills a ring buffer.

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> index dbb8bd434080a..714c7706f3551 100644
> --- a/drivers/net/can/loongson_canfd.c
> +++ b/drivers/net/can/loongson_canfd.c

[ ... ]

> @@ -524,13 +544,150 @@ static void loongson_canfd_set_txbuf_cmd(struct net_device *ndev,
> +static u32 loongson_canfd_get_rxdata_in_dma(struct loongson_canfd_priv *priv)
> +{
> +	u32 data = 0;
> +
> +	data = priv->rx_buf[LOONGSON_CANFD_RXDMA_NUM - priv->last_res--];

[Severity: Medium]
Does reading from the DMA buffer in loongson_canfd_get_rxdata_in_dma()
require a read memory barrier? On weakly ordered architectures like
LoongArch, the CPU might speculatively prefetch the coherent DMA buffer
before the MMIO status check in loongson_canfd_get_rxbuf_empty_in_dma()
confirms the data is ready. Could a dma_rmb() be needed to order this
memory read after the MMIO status check?

> +	if (!priv->last_res)
> +		priv->last_res = LOONGSON_CANFD_RXDMA_NUM;
> +
> +	return data;
> +}
> +
> +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: High]
Since priv->last_res wraps directly from 1 to LOONGSON_CANFD_RXDMA_NUM
(256) and never becomes 0, what happens when the cyclic DMA transfer
reaches the end of its period and state.residue is 0? Will this cause
the empty check to evaluate to false (since 256 != 0), making the
driver incorrectly believe data is available when the buffer is actually
empty?

[Severity: High]
Does this logic assume that a full frame is available as soon as the DMA
residue differs from last_res? Because the DMA engine writes word-by-word,
the empty check will return false as soon as a single 32-bit word is
written. Will the subsequent unconditional full frame read in
loongson_canfd_rx() read ahead of the DMA controller into stale memory,
parsing incorrect lengths and permanently losing synchronization with
the cyclic buffer?

[Severity: Medium]
Is there a mechanism to detect if the DMA write pointer has lapped the
read pointer under high traffic? Since the driver only checks for exact
equality of the read and write pointers (last_res == residue / 4), it
seems it cannot distinguish between an empty buffer and a buffer that
has been lapped by exactly 256 words. Will this lead to the CPU reading
from the middle of an overwritten frame without noticing?

[ ... ]

> @@ -957,14 +1117,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 < 2)
>  		return -EAGAIN;
>  
>  	dbcnt = (rwcnt - 2) * LOONGSON_CANFD_DW_BYTE;
>  	if (dbcnt > CANFD_MAX_DLEN)
>  		return -EAGAIN;

[Severity: High]
This is a pre-existing issue, but by returning -EAGAIN early in
loongson_canfd_rx() when rwcnt < 2 or dbcnt > CANFD_MAX_DLEN, does the
driver fail to drain the remaining invalid payload words from the RX
source? If the payload is not consumed, will the next read attempt to
parse the leftover payload as the metadata headers for the next frame,
permanently desynchronizing the RX stream? This seems easier to trigger
now with the new DMA race conditions.

[ ... ]

>  	/* 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 a pre-existing issue, but does this loop cause a heap buffer
overflow when receiving Classical CAN frames? In loongson_canfd_rx(),
for Classical CAN frames (is_can_fd == false), the allocated skb only
has an 8-byte data array. However, the earlier bounds check only validates
that dbcnt <= CANFD_MAX_DLEN (64). Could this write up to 56 bytes past
the end of the can_frame into skb_shared_info, potentially leading to
memory corruption?

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