Re: [PATCH v2 2/2] can: at91_can: add missing can_rx_offload_del() in at91_can_remove()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver ignores the return value of `can_rx_offload_add_timestamp()`, leading to a kernel panic upon allocation failure, and also misses cleanup in the probe error path.
--

commit 480793d438a773b528d044a7ee532ec7fbf0bc18
Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

can: at91_can: add missing can_rx_offload_del() in at91_can_remove()

at91_can_probe() sets up rx-offload via can_rx_offload_add_timestamp(),
but at91_can_remove() never calls can_rx_offload_del(). The NAPI
instance added by can_rx_offload_add_timestamp() is therefore never
removed on unbind.

[ ... ]

> diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
> index 58da323f14d7c..3ee908817add8 100644
> --- a/drivers/net/can/at91_can.c
> +++ b/drivers/net/can/at91_can.c
> @@ -1165,6 +1165,8 @@ static void at91_can_remove(struct platform_device *pdev)
>  
>  	unregister_netdev(dev);
>  
> +	can_rx_offload_del(&priv->offload);
> +

[Severity: High]
This isn't a bug introduced by this patch, but while fixing this missing
cleanup in at91_can_remove(), is there a similar leak in the at91_can_probe()
error path?

If register_candev() fails during probe, the exit_free error path is taken
but it misses calling can_rx_offload_del(). Could this leave a dangling NAPI
instance and leak the per-CPU queues?

drivers/net/can/at91_can.c:at91_can_probe() {
	...
	err = register_candev(dev);
	if (err) {
		dev_err(&pdev->dev, "registering netdev failed\n");
		goto exit_free;
	}
	...
 exit_free:
	free_candev(dev);
	...
}

Additionally, the return value of can_rx_offload_add_timestamp() in
at91_can_probe() is ignored.

If the allocation fails under memory pressure and returns an error like
-ENOMEM, the probe continues without allocating the per-CPU queues. When a
hardware interrupt eventually fires:

drivers/net/can/dev/rx-offload.c:can_rx_offload_irq_offload_timestamp() {
	...
	irq_queue = get_cpu_ptr(offload->skb_irq_queue);
	...
	__skb_queue_add_sort(irq_queue, skb, can_rx_offload_compare);
}

Could this result in passing an uninitialized NULL skb_irq_queue pointer to
get_cpu_ptr(), producing an invalid memory address and causing a fatal page
fault when dereferenced by __skb_queue_add_sort()?

Does at91_can_probe() need to check the return value of
can_rx_offload_add_timestamp() and handle the allocation error?

>  	iounmap(priv->reg_base);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901083927.316283-1-ciprianmarian.costea@oss.nxp.com?part=2