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

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Integer underflow and missing bounds check in the RX path allows out-of-bounds heap writes.
- [High] NAPI poll routine violates NAPI protocol on early exit, stranding the polling state machine.
- [High] Clearing the TX interrupt status after checking TX buffers causes a TOCTOU race condition that permanently loses completions.
- [High] Unhandled hardware interrupts and missing status clears trap the driver in an infinite ISR loop.
- [Medium] `loongson_canfd_start_xmit` silently drops packets instead of returning `NETDEV_TX_BUSY` when buffers are transiently unready.
- [Medium] Data race on `ndev->stats.rx_errors` due to concurrent modification from hardirq and softirq contexts.
- [Low] Useless `processed_mask` check in `loongson_canfd_tx_interrupt`.
--

--- Patch [1]: [PATCH v6 1/2] can: Add Loongson CAN-FD controller driver ---
commit 5f440f327c688c271e3208983fc49fe2d7c3dd2d
Author: Binbin Zhou <[email protected]>

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..211133258b083
> --- /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)
> +{
[ ... ]
> +	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?");
> +		dev_kfree_skb_any(skb);
> +		ndev->stats.tx_dropped++;
> +		return NETDEV_TX_OK;
> +	}

[Severity: Medium]
Is it correct to drop the packet and return NETDEV_TX_OK here?

If the loop falls through because all buffers have active tx_bs states, 
buf_id remains 0. Insertion will then fail. Should this instead stop the 
queue and return NETDEV_TX_BUSY to avoid dropping packets during transient 
buffer states?

[ ... ]
> +static int loongson_canfd_rx(struct net_device *ndev)
> +{
[ ... ]
> +	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 this result in an integer underflow if rwcnt is 1? 

It appears dbcnt operates without a bounds check against the SKB allocation 
size. If the hardware provides rwcnt < 2, dbcnt could underflow to a huge 
32-bit integer, leading to a buffer overflow when writing data to cfd->data.

[ ... ]
> +static void loongson_canfd_err_interrupt(struct net_device *ndev, u32 isr)
> +{
[ ... ]
> +	/* Check for Bus Error interrupt */
> +	if (isr & REG_INT_STAT_BEI) {
> +		mask |= REG_INT_STAT_BEI;
> +		netdev_err(ndev, "Bus Error interrupt\n");
> +		priv->can.can_stats.bus_error++;
> +		stats->rx_errors++;

[Severity: Medium]
Could this cause a data race on ndev->stats.rx_errors?

This is executed in hardirq context, while the same counter is also 
incremented in softirq context in loongson_canfd_rx_napi(). Non-atomic 
increments across these contexts might lead to torn updates or lost counts.

[ ... ]
> +static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
> +{
[ ... ]
> +	while (!rxbuf_is_empty && work_done < quota && ret != -EAGAIN) {
> +		ret = loongson_canfd_rx(ndev);
> +		work_done++;
> +		rxbuf_is_empty = loongson_canfd_rxbuf_empty(priv);
> +	}
[ ... ]
> +	if (rxbuf_is_empty) {
> +		if (napi_complete_done(napi, work_done)) {

[Severity: High]
What happens if the while loop exits early due to ret == -EAGAIN but 
rxbuf_is_empty is false? 

This skips napi_complete_done() even though work_done < quota, which 
leaves the NAPI state stuck in NAPI_STATE_SCHED but off the poll list, 
potentially stalling the RX path permanently.

[ ... ]
> +static void loongson_canfd_tx_interrupt(struct net_device *ndev)
> +{
[ ... ]
> +	for (buf_id = 0; buf_id < LOONGSON_CANFD_TXBUF_NUM; buf_id++) {
> +		if (processed_mask & BIT(buf_id))
> +			continue;

[Severity: Low]
Is this processed_mask check needed?

Since buf_id strictly increments from 0 to 7 in this loop, the bit for the 
current buf_id will always be clear when evaluated.

[ ... ]
> +	if (processed_mask) {
> +		/*
> +		 * 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);
> +	}

[Severity: High]
Could clearing the TX interrupt status at the end of the loop cause a race 
condition? 

If a different TX buffer completes transmission after it was checked by the 
loop but before the REG_INT_STAT_TXBHCI bit is cleared, the software clear 
might wipe out the new hardware assertion, permanently losing the completion 
for that buffer.

[ ... ]
> +static irqreturn_t loongson_canfd_interrupt(int irq, void *dev_id)
> +{
[ ... ]
> +	for (unsigned int irq_loops = 0; irq_loops < 10000; irq_loops++) {
> +		/* Get the interrupt status */
> +		regmap_read(priv->regmap, LOONGSON_CANFD_INT_STAT, &isr);

[Severity: High]
Is there a risk of an endless loop here?

If the hardware asserts an unhandled interrupt bit (like DOI or BSI), or if 
TXBHCI is set but skipped because processed_mask == 0 in the TX handler, 
the bit will remain set in INT_STAT. The loop will spin 10,000 times, and 
then loongson_canfd_disable_interrupts() will permanently kill the 
interface.

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