Re: [PATCH can-next] can: m_can: switch to rx-offload implementation

[sashiko-bot@kernel.org]

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

New issues:
- [High] Double counting of received packets and bytes, alongside a data race on network statistics between HardIRQ and SoftIRQ contexts.
- [High] Processing RX frames directly in hardirq with a strict, non-replenishable quota can leave packets stranded indefinitely in the hardware FIFO.
- [Low] The napi_struct is no longer used but was left in struct m_can_classdev.

Pre-existing issues:
- [High] The driver teardown path uses the non-blocking `hrtimer_try_to_cancel()` to stop the polling/coalescing timer, leading to unclocked hardware accesses and Use-After-Free vulnerabilities.
- [High] The teardown sequence in m_can_close leaks socket buffers and creates a guaranteed NULL pointer dereference when flushing the transmission workqueue.
- [High] Uninitialized stack variable `frame_len` corrupts Byte Queue Limits (BQL) accounting.
--

--- Patch [1]: [PATCH can-next] can: m_can: switch to rx-offload implementation ---
commit 7f375f1ac288dad1360ddf2dd262a4c708abc626
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: m_can: switch to rx-offload implementation

This patch converts the m_can driver to unconditionally use rx-offload
for all devices, calling m_can_rx_handler() directly from the IRQ
handler instead of using NAPI.

> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index eb856547ae7df..8175b030257b4 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -530,26 +530,17 @@ static void m_can_clean(struct net_device *net)
[ ... ]
>  static void m_can_receive_skb(struct m_can_classdev *cdev,
>  			      struct sk_buff *skb,
>  			      u32 timestamp)
>  {
> -	if (cdev->is_peripheral) {
> -		struct net_device_stats *stats = &cdev->net->stats;
> -		int err;
> +	struct net_device_stats *stats = &cdev->net->stats;
> +	int err;
>  
> -		err = can_rx_offload_queue_timestamp(&cdev->offload, skb,
> -						     timestamp);
> -		if (err)
> -			stats->rx_fifo_errors++;
> -	} else {
> -		netif_receive_skb(skb);
> -	}
> +	err = can_rx_offload_queue_timestamp(&cdev->offload, skb,
> +					     timestamp);

[Severity: High]
Does this change introduce a data race and double counting for network stats?

When the mmio devices are switched to use rx-offload, m_can_read_fifo()
manually increments the stats in hardirq context before calling
m_can_receive_skb():

m_can_read_fifo()
  stats->rx_bytes += cf->len;
  stats->rx_packets++;
  m_can_receive_skb(..., skb, ...);

The SKB is then queued to the offload queue. When the rx-offload NAPI poll
runs in softirq context, it unconditionally increments the exact same
counters again:

can_rx_offload_napi_poll()
  stats->rx_packets++;
  stats->rx_bytes += cf->len;

Does this completely inflate the RX statistics, as well as cause a concurrent
data race between hardirq and softirq contexts updating dev->stats?

Additionally, wouldn't TX echo packets queued via
can_rx_offload_get_echo_skb_queue_timestamp() also be dequeued by the poll
routine, erroneously counting transmitted frames as RX packets?

[ ... ]
> @@ -1065,32 +1052,6 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
[ ... ]
>  static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
>  					  unsigned int msg_mark, u32 timestamp)
>  {
> @@ -1098,14 +1059,11 @@ static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
>  	struct net_device_stats *stats = &dev->stats;
>  	unsigned int frame_len;
>  
> -	if (cdev->is_peripheral)
> -		stats->tx_bytes +=
> -			can_rx_offload_get_echo_skb_queue_timestamp(&cdev->offload,
> -								    msg_mark,
> -								    timestamp,
> -								    &frame_len);
> -	else
> -		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, &frame_len);
> +	stats->tx_bytes +=
> +		can_rx_offload_get_echo_skb_queue_timestamp(&cdev->offload,
> +							    msg_mark,
> +							    timestamp,
> +							    &frame_len);
>  
>  	stats->tx_packets++;
>  
>  	return frame_len;

[Severity: High]
This is a pre-existing issue, but is it possible for an uninitialized stack
variable to corrupt BQL accounting?

In m_can_tx_update_stats(), frame_len is passed by reference to
can_rx_offload_get_echo_skb_queue_timestamp(). If the echo SKB is missing
or invalid, the helper function returns 0 immediately and does not initialize
frame_len.

Will this return uninitialized stack garbage that gets passed to
netdev_completed_queue() in m_can_finish_tx(), potentially stalling the TX
queue by corrupting Byte Queue Limits accounting?

[ ... ]
> @@ -1265,21 +1223,10 @@ static int m_can_interrupt_handler(struct m_can_classdev *cdev)
[ ... ]
>  	if (ir & (IR_RF0N | IR_RF0W | IR_ERR_ALL_30X)) {
> -		cdev->irqstatus = ir;
> -		if (!cdev->is_peripheral) {
> -			m_can_disable_all_interrupts(cdev);
> -			napi_schedule(&cdev->napi);
> -		} else {
> -			ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
> -			if (ret < 0)
> -				return ret;
> -		}
> +		ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
> +		if (ret < 0)
> +			return ret;
>  	}

[Severity: High]
Could directly calling m_can_rx_handler() from the hardirq handler cause
packets to be stranded indefinitely?

The handler is now called with a strict quota of NAPI_POLL_WEIGHT. If the FIFO
contains more messages than the remaining quota (which can shrink if work_done
is consumed by state errors), the read loop in m_can_do_rx_poll() breaks
early.

Because the IR_RF0N hardware interrupt is edge-triggered (only set when a new
message is written) and the NAPI automatic rescheduling is removed, will the
remaining packets sit in the hardware FIFO indefinitely until another frame
happens to arrive on the bus?

[ ... ]
> @@ -1846,11 +1791,10 @@ static int m_can_close(struct net_device *dev)
>  	if (cdev->is_peripheral) {
>  		destroy_workqueue(cdev->tx_wq);
>  		cdev->tx_wq = NULL;
> -		can_rx_offload_disable(&cdev->offload);
> -	} else {
> -		napi_disable(&cdev->napi);
>  	}

[Severity: High]
This is a pre-existing issue, but can the teardown sequence in m_can_close()
trigger a null pointer dereference?

During m_can_close(), m_can_clean() iterates through the tx_ops and sets
the skb pointers to NULL:

m_can_clean()
  cdev->tx_ops[i].skb = NULL;

Immediately after, destroy_workqueue(cdev->tx_wq) is called, which drains the
queue and executes pending tasks. When the flushed m_can_tx_work_queue() task
runs, it passes the nullified skb pointer to the tx handler:

m_can_tx_work_queue()
  struct sk_buff *skb = op->skb;
  m_can_tx_handler(cdev, skb);

Will this result in a null pointer dereference since the tx handler
unconditionally accesses skb->data? Also, is there a memory leak since
m_can_clean() clears the pointer without freeing the SKB memory?

[Severity: High]
This is a pre-existing issue, but does the teardown path safely stop the
polling timer?

Looking at the teardown sequence in m_can_close(), the interface is shut
down and interrupts are disabled, which relies on hrtimer_try_to_cancel() in
m_can_disable_all_interrupts():

m_can_close()
  m_can_stop()
    m_can_disable_all_interrupts()
      hrtimer_try_to_cancel(&cdev->hrtimer);

If the timer callback is actively running on another CPU,
hrtimer_try_to_cancel() returns -1 without waiting for the callback to finish.
The teardown path then continues to free data structures, disable hardware
clocks, and power off the PHY.

Could this lead to a use-after-free and synchronous bus fault when the
concurrently executing timer callback accesses unmapped or unclocked MMIO
regions?

Also, m_can_class_unregister() appears to lack explicit timer teardown, which
might leave the timer active during module unload.

[ ... ]
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 4743342b2fba3..ca5833650dbfb 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -96,7 +96,6 @@ struct m_can_classdev {
>  	const struct m_can_ops *ops;
>  
>  	int version;
> -	u32 irqstatus;
>  
>  	int pm_clock_support;
>  	int pm_wake_source;

[Severity: Low]
Is it intentional to keep the napi structure in m_can_classdev?

The patch removes all manual napi handling from the driver, but it looks like
struct napi_struct napi; is still present in the struct.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-m_can-rx-offload-v1-1-af3efa8e4272@pengutronix.de?part=1