Re: [PATCH v5 1/2] can: Add Loongson CAN-FD controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing bounds check on the hardware-provided frame length (`rwcnt`) leads to a kernel heap buffer overflow when reading RX payloads.
- [High] NAPI polling is permanently stalled if SKB allocation fails in the RX path due to skipping `napi_complete_done()`.
- [High] Spurious double processing of TX completions and infinite loop potential due to polling MMIO status immediately after issuing a clear command.
- [Medium] The driver advertises support for `CAN_CTRLMODE_3_SAMPLES` but fails to implement it, leading to silent failures to honor user configuration.
- [Medium] The driver incorrectly resets hardware error counters immediately upon entering bus-off state, bypassing the CAN framework's restart delay mechanism.
- [Medium] Calling `kfree_skb()` with local interrupts disabled.
- [Low] A local variable declaration immediately following a switch case label violates C syntax standards and causes a build failure.
--

--- Patch [1]: [PATCH v5 1/2] can: Add Loongson CAN-FD controller driver ---
commit e34dfafbc093b1981c79c97332825864c52e75bd
Author: Binbin Zhou <zhoubinbin@loongson.cn>
Subject: can: Add Loongson CAN-FD controller driver

Add a new driver for the Loongson CAN-FD controller, which is integrated
into Loongson-2K series SoCs. The controller supports both CAN 2.0 and
CAN-FD protocols, with up to 8 TX buffers and a flexible RX buffer.

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> new file mode 100644
> index 0000000000000..0e85005a9bb12
> --- /dev/null
> +++ b/drivers/net/can/loongson_canfd.c

[ ... ]

> +static netdev_tx_t loongson_canfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +	struct loongson_canfd_priv *priv = netdev_priv(ndev);
> +	u32 buf_id = 0, tx_stat, i = 0;
> +	unsigned long tx_brp;
> +	u16 tx_bs;
> +
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +
> +	guard(spinlock_irqsave)(&priv->tx_lock);
> +
> +	if (unlikely(!loongson_canfd_txbuf_freed(priv))) {
> +		netif_stop_queue(ndev);
> +		netdev_err(ndev, "BUG!, no TXB free when queue awake!\n");
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	regmap_read(priv->regmap, LOONGSON_CANFD_TX_STAT, &tx_stat);
> +	tx_brp = FIELD_GET(REG_TX_STAT_BRP, tx_stat);
> +	tx_bs = FIELD_GET(REG_TX_STAT_BS, tx_stat);
> +
> +	for_each_clear_bit(i, &tx_brp, LOONGSON_CANFD_TXBUF_NUM) {
> +		u32 transmission_complete_mask = 0x3 << (i * 2);
> +
> +		if (!(tx_bs & transmission_complete_mask)) {
> +			buf_id = i;
> +			break;
> +		}
> +	}
> +
> +	if (!loongson_canfd_insert_frame(priv, cf, buf_id, can_is_canfd_skb(skb))) {
> +		netdev_err(ndev, "TXNF set but cannot insert frame into TXTB! HW Bug?");
> +		kfree_skb(skb);
[Severity: Medium]
Is it safe to call kfree_skb() here with local hardware interrupts disabled?

Since this block of code runs with priv->tx_lock held via 
guard(spinlock_irqsave), could we use dev_kfree_skb_any() instead to avoid 
potentially sleeping or violating atomic context APIs?

> +		ndev->stats.tx_dropped++;
> +		return NETDEV_TX_OK;
> +	}

[ ... ]

> +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;
> +	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);
> +
> +	/* Number of characters received */
> +	rwcnt = FIELD_GET(REG_FRAME_META1_RWCNT, meta1);
> +	if (!rwcnt)
> +		return -EAGAIN;
> +	dbcnt = (rwcnt - 2) * LOONGSON_CANFD_DW_BYTE;
[Severity: High]
Can dbcnt underflow if the hardware provides a value of 1 for rwcnt?

If a malfunctioning or malicious CAN controller provides an rwcnt of 1, 
the subtraction will underflow, causing dbcnt to become a very large number. 
This would lead to a kernel heap buffer overflow when the subsequent code 
reads payload data into cfd->data.

Could we add a bounds check to ensure rwcnt >= 2 and dbcnt <= CANFD_MAX_DLEN?

> +
> +	/* Flexible Data-rate Format */

[ ... ]

> +static void loongson_canfd_err_interrupt(struct net_device *ndev, u32 isr)
> +{
> +	struct loongson_canfd_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	struct can_berr_counter bec;
> +	u32 capt, alc, mask = 0;
> +	enum can_state state;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +
> +	loongson_canfd_get_bec(priv, &bec);
> +
> +	state = loongson_canfd_read_fault_state(ndev);

[ ... ]

> +	if ((isr & REG_INT_STAT_FCSI) || (isr & REG_INT_STAT_EWLI)) {
> +		netdev_info(ndev, "state changes from %s to %s\n",
> +			    can_get_state_str(priv->can.state), can_get_state_str(state));
> +
> +		if (priv->can.state == state)
> +			netdev_warn(ndev, "cur and pre state is the same!(miss intr?)\n");
> +
> +		mask |= REG_INT_STAT_FCSI | REG_INT_STAT_EWLI;
> +		priv->can.state = state;
> +		switch (state) {
> +		case CAN_STATE_BUS_OFF:
> +			priv->can.can_stats.bus_off++;
> +			if (priv->can.restart_ms)
> +				regmap_write(priv->regmap, LOONGSON_CANFD_CMD,
> +					     REG_CMD_ERCRST);
[Severity: Medium]
Does resetting the hardware error counters immediately upon entering bus-off
state bypass the CAN framework's restart delay mechanism?

Clearing the hardware error counters here forces the controller to attempt
recovery instantly, which circumvents the software-defined isolation delay
(restart_ms) that is supposed to keep the device off the bus before
can_restart() eventually re-enables it.

> +
> +			can_bus_off(ndev);
> +			if (skb)
> +				cf->can_id |= CAN_ERR_BUSOFF;
> +			break;

[ ... ]

> +static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
> +{
> +	struct net_device *ndev = napi->dev;
> +	struct loongson_canfd_priv *priv = netdev_priv(ndev);
> +	int work_done = 0, ret = 1;
> +	bool rxbuf_is_empty;
> +	u32 sts;
> +
> +	rxbuf_is_empty = loongson_canfd_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);
> +	}
> +
> +	/* Check for RX FIFO Overflow */

[ ... ]

> +	if (rxbuf_is_empty && ret != 0) {
> +		if (napi_complete_done(napi, work_done)) {
[Severity: High]
Can NAPI polling stall permanently if SKB allocation fails in the RX path?

If alloc_can_skb() fails due to memory pressure, loongson_canfd_rx() returns
0. The NAPI while loop breaks early because ret > 0 evaluates to false. 

Because of the condition above (ret != 0), napi_complete_done() is then 
skipped. This leaves the NAPI instance stuck in the NAPI_STATE_SCHED state 
without unmasking the RX interrupt, permanently deadlocking the receive path 
until the interface is brought down and back up.

> +			/*
> +			 * Clear and enable RBNEI. It is level-triggered,
> +			 * so there is no race condition.
> +			 */
> +			regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, REG_INT_STAT_RBNEI);

[ ... ]

> +static void loongson_canfd_tx_interrupt(struct net_device *ndev)
> +{
> +	struct loongson_canfd_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &ndev->stats;
> +	enum loongson_canfd_txbuf_sts sts;
> +	bool some_buffers_processed;
> +	u32 buf_id;
> +
> +	do {
> +		some_buffers_processed = false;
> +
> +		guard(spinlock_irqsave)(&priv->tx_lock);
> +
> +		while ((buf_id = loongson_canfd_get_txbuf_id(priv, &sts)) <
> +			LOONGSON_CANFD_TXBUF_NUM) {
> +			switch (sts) {
> +			case TX_BS_VALID:
> +				stats->tx_bytes += can_get_echo_skb(ndev, buf_id, NULL);
> +				stats->tx_packets++;
> +				break;
> +			case TX_BS_FAIL:
> +				u32 cnt;
[Severity: Low]
Does this local variable declaration directly following a label violate C
syntax rules?

Under standard C11 rules (gnu11), a label must be attached to a statement, 
and a declaration is not considered a statement. This can cause compilation 
to fail with modern GCC versions. 

Could we move this declaration outside the switch block, or wrap the logic 
for this case inside braces?

> +				/*
> +				 * This indicated that retransmit limit has been reached.
> +				 * Obviously we should not echo the frame, but also not indicate any
> +				 * kind of error. If desired, it was already reported (possible
> +				 * multiple times) on each arbitration lost.
> +				 */

[ ... ]

> +			loongson_canfd_set_txbuf_cmd(ndev, TXT_CMD_SR_CLEAN, buf_id);
> +			some_buffers_processed = true;
> +		}
> +
> +		/*
> +		 * If no buffers were processed this time, we cannot clear - that would introduce
> +		 * a race condition.
> +		 */
> +		if (some_buffers_processed) {
> +			/*
> +			 * Clear the interrupt again. We do not want to receive again interrupt
> +			 * for the buffer already handled. If it is the last finished one then
> +			 * it would cause log of spurious interrupt.
> +			 */
> +			regmap_write(priv->regmap, LOONGSON_CANFD_INT_STAT, REG_INT_STAT_TXBHCI);
> +		}
> +	} while (some_buffers_processed);
[Severity: High]
Could this loop result in spurious double-processing of TX completions?

If the MMIO write to clear the status bit (TXT_CMD_SR_CLEAN) takes time to
propagate through the hardware, the next immediate iteration of the loop 
could read the old status from LOONGSON_CANFD_TX_STAT.

This would cause the driver to process the exact same TX buffer multiple 
times, inflating tx_packets or tx_dropped and spamming the logs with warnings 
until the hardware finally updates the register.

> +
> +	guard(spinlock_irqsave)(&priv->tx_lock);

[ ... ]

> +static int loongson_canfd_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct loongson_canfd_priv *priv;
> +	struct net_device *ndev;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	void __iomem *base;
> +	int ret = 0, irq;
> +	u32 clk_rate;

[ ... ]

> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY |
> +				       CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_ONE_SHOT |
[Severity: Medium]
Does the driver actually support CAN_CTRLMODE_3_SAMPLES in hardware?

The driver advertises 3-samples mode here, allowing userspace to request it, 
but functions like loongson_canfd_set_bittiming() and loongson_canfd_set_btr() 
do not appear to check for this flag or program the hardware appropriately 
to use multiple samples. 

Could this lead to silent failures to honor the user's requested configuration?

> +				       CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_FD |
> +				       CAN_CTRLMODE_PRESUME_ACK | CAN_CTRLMODE_FD_NON_ISO |
> +				       CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_CC_LEN8_DLC;

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