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

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

Pre-existing issues:
- [Critical] Closing a peripheral interface with pending TX causes a NULL pointer dereference and leaks SKBs due to incorrect teardown ordering.
- [High] Missing synchronous hrtimer cancellation in device teardown leads to concurrent MMIO access without clocks and use-after-free.
--

--- Patch [1]: [PATCH can-next v2] can: m_can: switch to rx-offload implementation ---
commit 5cf910484c596d7ed3917c0539c9c6c2d33f1e19
Author: Marc Kleine-Budde <[email protected]>

can: m_can: switch to rx-offload implementation

This patch unconditionally configures the driver to use rx-offload in 
m_can_rx_handler() from the IRQ handler instead of using NAPI for
memory-mapped devices. It removes manual NAPI handling from the driver.

> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index eb856547ae7df..866c4b501dadd 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c

[ ... ]

> @@ -1846,11 +1782,10 @@ static int m_can_close(struct net_device *dev)
>  	if (cdev->is_peripheral) {
>  		destroy_workqueue(cdev->tx_wq);
>  		cdev->tx_wq = NULL;

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference and memory leak here during teardown.

Earlier in m_can_close(), m_can_clean() iterates over pending TX operations
and sets their skb pointers to NULL:

m_can_clean()
    if (!cdev->tx_ops[i].skb)
        continue;
    net->stats.tx_errors++;
    cdev->tx_ops[i].skb = NULL;

When destroy_workqueue(cdev->tx_wq) runs right after, it synchronously
executes any pending tx work items. m_can_tx_work_queue() retrieves the
NULL skb and passes it to m_can_tx_handler(), which dereferences it
unconditionally via (struct canfd_frame *)skb->data, causing a panic.

Since the SKBs are also not freed when set to NULL, they appear to leak.
Does m_can_clean() need to free the SKB instead of just nulling it out,
and does the work queue need to handle a NULL skb gracefully?

> -		can_rx_offload_disable(&cdev->offload);
> -	} else {
> -		napi_disable(&cdev->napi);
>  	}
>  
> +	can_rx_offload_disable(&cdev->offload);
> +
>  	close_candev(dev);
>  
>  	reset_control_assert(cdev->rst);

[ ... ]

> @@ -2589,8 +2515,7 @@ EXPORT_SYMBOL_GPL(m_can_class_register);
>  void m_can_class_unregister(struct m_can_classdev *cdev)
>  {
>  	unregister_candev(cdev->net);
> -	if (cdev->is_peripheral)
> -		can_rx_offload_del(&cdev->offload);
> +	can_rx_offload_del(&cdev->offload);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I noticed the hrtimer might
not be fully shut down during device teardown.

If a user disables the CAN interface or unbinds the driver while the
hrtimer (m_can_polling_timer or m_can_coalescing_timer) is executing on
another CPU, we could hit a use-after-free or bus fault.

m_can_disable_all_interrupts() calls hrtimer_try_to_cancel(), which returns
without stopping the timer if it is currently running. The timer could then
execute concurrently after hardware clocks are disabled via m_can_clk_stop()
in m_can_close(), or after the device is unregistered and freed.

Should there be a synchronous cancel like hrtimer_cancel() during the
teardown path to ensure the timer is fully stopped before disabling clocks
and freeing memory?

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