[PATCH v2 0/2] can: rx-offload: make skb_irq_queue per-CPU

[PATCH v2 0/2] can: rx-offload: make skb_irq_queue per-CPU

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

can_rx_offload keeps a lockless irq_queue that the IRQ handlers fill and
that is later spliced under skb_queue.lock into the NAPI-facing skb_queue.
This works as long as a single context fills the irq_queue. flexcan with
FLEXCAN_QUIRK_SECONDARY_MB_IRQ and mcf5441x use two mailbox IRQ lines. When
those are affined to different CPUs the two handlers can enqueue into the
same list at the same time and corrupt it.

This series:
  1. Makes the irq_queue per-CPU so the handlers no longer share a list.
  2. Adds the can_rx_offload_del() that at91_can was missing; without it
the per-CPU allocation is leaked on unbind.

Changes since v1:

- The enqueue helpers used this_cpu_ptr() without disabling preemption.
  All four enqueue helpers now use get_cpu_ptr()/put_cpu_ptr().
- Guard can_rx_offload_del() against skb_irq_queue == NULL.
- Fix 'at91_can' memory leak by adding missing 'can_rx_offload_del'.

Ciprian Marian Costea (2):
  can: rx-offload: make skb_irq_queue per-CPU
  can: at91_can: add missing can_rx_offload_del() in at91_can_remove()

 drivers/net/can/at91_can.c       |  2 +
 drivers/net/can/dev/rx-offload.c | 83 ++++++++++++++++++++++++++------
 include/linux/can/rx-offload.h   |  2 +-
 3 files changed, 72 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] can: rx-offload: make skb_irq_queue per-CPU

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

skb_irq_queue is filled by the IRQ handlers using the lockless
__skb_queue_add_sort() / __skb_queue_tail() helpers and later spliced
into skb_queue under skb_queue.lock by can_rx_offload_irq_finish() and
can_rx_offload_threaded_irq_finish().

This is only safe while a single context fills skb_irq_queue. FlexCAN
on NXP S32G2 (FLEXCAN_QUIRK_SECONDARY_MB_IRQ) uses two mailbox IRQ
lines, one for MB0-7 and one for MB8-63; MCF5441X similarly splits its
mailbox interrupt. When these lines are affined to different CPUs both
handlers can run at the same time and enqueue into the same sk_buff_head
concurrently, corrupting its list.

Allocate skb_irq_queue per-CPU so the handlers no longer share a list,
keeping the enqueue path lock-free. Access the per-CPU queue via
get_cpu_ptr()/put_cpu_ptr() in the enqueue helpers: this disables
preemption around the lockless __skb_queue_*() operation.

can_rx_offload_irq_finish() runs in the same context as its enqueues and
splices this_cpu_ptr(). can_rx_offload_threaded_irq_finish() may have
been migrated after its enqueues, so it splices every possible CPU's
queue; this is safe because each per-CPU queue has a single producer and
that producer runs with preemption disabled, so it cannot race the
splice.

Cross-line frames are now sorted by timestamp only within a CPU's queue
and appended across CPUs on splice; each skb keeps its own timestamp.

Fixes: c757096ea103 ("can: rx-offload: add skb queue for use during ISR")
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/dev/rx-offload.c | 83 ++++++++++++++++++++++++++------
 include/linux/can/rx-offload.h   |  2 +-
 2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index 46e7b6db4a1e..649bfda08b65 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -7,6 +7,7 @@
 
 #include <linux/can/dev.h>
 #include <linux/can/rx-offload.h>
+#include <linux/percpu.h>
 
 struct can_rx_offload_cb {
 	u32 timestamp;
@@ -175,9 +176,18 @@ can_rx_offload_offload_one(struct can_rx_offload *offload, unsigned int n)
 int can_rx_offload_irq_offload_timestamp(struct can_rx_offload *offload,
 					 u64 pending)
 {
+	struct sk_buff_head *irq_queue;
 	unsigned int i;
 	int received = 0;
 
+	/*
+	 * get_cpu_ptr() disables preemption so that the lockless
+	 * __skb_queue_*() below operate on the current CPU's queue without
+	 * racing a migration. This also keeps this_cpu_ptr() valid when a
+	 * driver enqueues from a preemptible (threaded IRQ) context.
+	 */
+	irq_queue = get_cpu_ptr(offload->skb_irq_queue);
+
 	for (i = offload->mb_first;
 	     can_rx_offload_le(offload, i, offload->mb_last);
 	     can_rx_offload_inc(offload, &i)) {
@@ -190,20 +200,25 @@ int can_rx_offload_irq_offload_timestamp(struct can_rx_offload *offload,
 		if (IS_ERR_OR_NULL(skb))
 			continue;
 
-		__skb_queue_add_sort(&offload->skb_irq_queue, skb,
+		__skb_queue_add_sort(irq_queue, skb,
 				     can_rx_offload_compare);
 		received++;
 	}
 
+	put_cpu_ptr(offload->skb_irq_queue);
+
 	return received;
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_irq_offload_timestamp);
 
 int can_rx_offload_irq_offload_fifo(struct can_rx_offload *offload)
 {
+	struct sk_buff_head *irq_queue;
 	struct sk_buff *skb;
 	int received = 0;
 
+	irq_queue = get_cpu_ptr(offload->skb_irq_queue);
+
 	while (1) {
 		skb = can_rx_offload_offload_one(offload, 0);
 		if (IS_ERR(skb))
@@ -211,10 +226,12 @@ int can_rx_offload_irq_offload_fifo(struct can_rx_offload *offload)
 		if (!skb)
 			break;
 
-		__skb_queue_tail(&offload->skb_irq_queue, skb);
+		__skb_queue_tail(irq_queue, skb);
 		received++;
 	}
 
+	put_cpu_ptr(offload->skb_irq_queue);
+
 	return received;
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_irq_offload_fifo);
@@ -222,6 +239,7 @@ EXPORT_SYMBOL_GPL(can_rx_offload_irq_offload_fifo);
 int can_rx_offload_queue_timestamp(struct can_rx_offload *offload,
 				   struct sk_buff *skb, u32 timestamp)
 {
+	struct sk_buff_head *irq_queue;
 	struct can_rx_offload_cb *cb;
 
 	if (skb_queue_len(&offload->skb_queue) >
@@ -233,8 +251,9 @@ int can_rx_offload_queue_timestamp(struct can_rx_offload *offload,
 	cb = can_rx_offload_get_cb(skb);
 	cb->timestamp = timestamp;
 
-	__skb_queue_add_sort(&offload->skb_irq_queue, skb,
-			     can_rx_offload_compare);
+	irq_queue = get_cpu_ptr(offload->skb_irq_queue);
+	__skb_queue_add_sort(irq_queue, skb, can_rx_offload_compare);
+	put_cpu_ptr(offload->skb_irq_queue);
 
 	return 0;
 }
@@ -268,13 +287,17 @@ EXPORT_SYMBOL_GPL(can_rx_offload_get_echo_skb_queue_timestamp);
 int can_rx_offload_queue_tail(struct can_rx_offload *offload,
 			      struct sk_buff *skb)
 {
+	struct sk_buff_head *irq_queue;
+
 	if (skb_queue_len(&offload->skb_queue) >
 	    offload->skb_queue_len_max) {
 		dev_kfree_skb_any(skb);
 		return -ENOBUFS;
 	}
 
-	__skb_queue_tail(&offload->skb_irq_queue, skb);
+	irq_queue = get_cpu_ptr(offload->skb_irq_queue);
+	__skb_queue_tail(irq_queue, skb);
+	put_cpu_ptr(offload->skb_irq_queue);
 
 	return 0;
 }
@@ -307,14 +330,15 @@ EXPORT_SYMBOL_GPL(can_rx_offload_get_echo_skb_queue_tail);
 
 void can_rx_offload_irq_finish(struct can_rx_offload *offload)
 {
+	struct sk_buff_head *irq_queue = this_cpu_ptr(offload->skb_irq_queue);
 	unsigned long flags;
 	int queue_len;
 
-	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
+	if (skb_queue_empty_lockless(irq_queue))
 		return;
 
 	spin_lock_irqsave(&offload->skb_queue.lock, flags);
-	skb_queue_splice_tail_init(&offload->skb_irq_queue, &offload->skb_queue);
+	skb_queue_splice_tail_init(irq_queue, &offload->skb_queue);
 	spin_unlock_irqrestore(&offload->skb_queue.lock, flags);
 
 	queue_len = skb_queue_len(&offload->skb_queue);
@@ -330,15 +354,29 @@ void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
 {
 	unsigned long flags;
 	int queue_len;
-
-	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
-		return;
-
+	int cpu;
+
+	/*
+	 * Splice every CPU's queue: unlike the non-threaded
+	 * can_rx_offload_irq_finish(), a threaded handler may be migrated
+	 * between the enqueue and this splice, so the frames may sit on a
+	 * different CPU's queue. This is only safe because a given per-CPU
+	 * queue has a single producer (the enqueue on that CPU is
+	 * non-preemptible), so no producer can race this splice.
+	 */
 	spin_lock_irqsave(&offload->skb_queue.lock, flags);
-	skb_queue_splice_tail_init(&offload->skb_irq_queue, &offload->skb_queue);
+	for_each_possible_cpu(cpu) {
+		struct sk_buff_head *irq_queue;
+
+		irq_queue = per_cpu_ptr(offload->skb_irq_queue, cpu);
+		skb_queue_splice_tail_init(irq_queue, &offload->skb_queue);
+	}
 	spin_unlock_irqrestore(&offload->skb_queue.lock, flags);
 
 	queue_len = skb_queue_len(&offload->skb_queue);
+	if (!queue_len)
+		return;
+
 	if (queue_len > offload->skb_queue_len_max / 8)
 		netdev_dbg(offload->dev, "%s: queue_len=%d\n",
 			   __func__, queue_len);
@@ -353,13 +391,21 @@ static int can_rx_offload_init_queue(struct net_device *dev,
 				     struct can_rx_offload *offload,
 				     unsigned int weight)
 {
+	int cpu;
+
 	offload->dev = dev;
 
 	/* Limit queue len to 4x the weight (rounded to next power of two) */
 	offload->skb_queue_len_max = 2 << fls(weight);
 	offload->skb_queue_len_max *= 4;
 	skb_queue_head_init(&offload->skb_queue);
-	__skb_queue_head_init(&offload->skb_irq_queue);
+
+	offload->skb_irq_queue = alloc_percpu(struct sk_buff_head);
+	if (!offload->skb_irq_queue)
+		return -ENOMEM;
+
+	for_each_possible_cpu(cpu)
+		__skb_queue_head_init(per_cpu_ptr(offload->skb_irq_queue, cpu));
 
 	netif_napi_add_weight(dev, &offload->napi, can_rx_offload_napi_poll,
 			      weight);
@@ -420,8 +466,17 @@ EXPORT_SYMBOL_GPL(can_rx_offload_enable);
 
 void can_rx_offload_del(struct can_rx_offload *offload)
 {
+	int cpu;
+
 	netif_napi_del(&offload->napi);
 	skb_queue_purge(&offload->skb_queue);
-	__skb_queue_purge(&offload->skb_irq_queue);
+
+	if (!offload->skb_irq_queue)
+		return;
+
+	for_each_possible_cpu(cpu)
+		__skb_queue_purge(per_cpu_ptr(offload->skb_irq_queue, cpu));
+
+	free_percpu(offload->skb_irq_queue);
 }
 EXPORT_SYMBOL_GPL(can_rx_offload_del);
diff --git a/include/linux/can/rx-offload.h b/include/linux/can/rx-offload.h
index d29bb4521947..1b9e2a8ab39a 100644
--- a/include/linux/can/rx-offload.h
+++ b/include/linux/can/rx-offload.h
@@ -20,7 +20,7 @@ struct can_rx_offload {
 					bool drop);
 
 	struct sk_buff_head skb_queue;
-	struct sk_buff_head skb_irq_queue;
+	struct sk_buff_head __percpu *skb_irq_queue;
 	u32 skb_queue_len_max;
 
 	unsigned int mb_first;
-- 
2.43.0

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

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

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.

This was benign while the offload's IRQ queue was embedded in struct
can_rx_offload, but it now leaks the per-CPU skb_irq_queue allocation.
Add the missing can_rx_offload_del() after unregister_netdev().

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/at91_can.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c
index 58da323f14d7..3ee908817add 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);
+
 	iounmap(priv->reg_base);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
2.43.0

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

[sashiko-bot]

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

Re: [PATCH v2 1/2] can: rx-offload: make skb_irq_queue per-CPU

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Thread migration during threaded IRQ handling and sequential per-CPU queue splicing destroys global CAN frame chronological ordering.
--

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

can: rx-offload: make skb_irq_queue per-CPU

This commit allocates skb_irq_queue per-CPU to prevent lockless queue
corruption when multiple mailbox IRQ lines are affined to different CPUs.
It removes cross-CPU timestamp sorting, instead sorting frames only within
each per-CPU queue and appending them across CPUs on splice.

> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> index 46e7b6db4a1ef..649bfda08b654 100644
> --- a/drivers/net/can/dev/rx-offload.c
> +++ b/drivers/net/can/dev/rx-offload.c

[ ... ]

> @@ -330,15 +354,29 @@ void can_rx_offload_threaded_irq_finish(struct can_rx_offload *offload)
>  {
>  	unsigned long flags;
>  	int queue_len;
> -
> -	if (skb_queue_empty_lockless(&offload->skb_irq_queue))
> -		return;
> -
> +	int cpu;
> +
> +	/*
> +	 * Splice every CPU's queue: unlike the non-threaded
> +	 * can_rx_offload_irq_finish(), a threaded handler may be migrated
> +	 * between the enqueue and this splice, so the frames may sit on a
> +	 * different CPU's queue. This is only safe because a given per-CPU
> +	 * queue has a single producer (the enqueue on that CPU is
> +	 * non-preemptible), so no producer can race this splice.
> +	 */
>  	spin_lock_irqsave(&offload->skb_queue.lock, flags);
> -	skb_queue_splice_tail_init(&offload->skb_irq_queue, &offload->skb_queue);
> +	for_each_possible_cpu(cpu) {
> +		struct sk_buff_head *irq_queue;
> +
> +		irq_queue = per_cpu_ptr(offload->skb_irq_queue, cpu);
> +		skb_queue_splice_tail_init(irq_queue, &offload->skb_queue);
> +	}
        ^^^^

[Severity: High]
Does splicing the per-CPU queues in fixed numerical order destroy the
chronological ordering of CAN frames if the threaded IRQ handler migrates?

In CAN drivers using threaded IRQ handlers (like mcp251xfd), bursts of
frames are processed in a loop. Because can_rx_offload_queue_timestamp()
and can_rx_offload_queue_tail() only disable preemption locally during the
actual enqueue operation, the kernel thread can be preempted and migrated
to a different CPU between processing frames.

This distributes sequential frames from the exact same CAN interface across
multiple per-CPU queues. By blindly splicing them into the global skb_queue
in strict numerical CPU order, if the thread migrated from a higher CPU
(e.g., CPU 1) to a lower CPU (e.g., CPU 0), the later frames on CPU 0 are
spliced before the earlier frames on CPU 1. 

Could this permanently invert their order and violate the strict FIFO
chronological ordering required by CAN bus protocols?

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

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

[Ciprian Marian Costea]

On 9/1/2026 11:49 AM, sashiko-bot@kernel.org wrote:
> 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);
> 

Good finding. Will fix in V3.

Ciprian