[PATCH] can: rx-offload: make skb_irq_queue per-CPU

[PATCH] 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 and enqueue via this_cpu_ptr() so the
handlers no longer share a list, keeping the enqueue path lock-free.
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 be
migrated, so it splices every possible CPU's queue.

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 | 51 ++++++++++++++++++++++++--------
 include/linux/can/rx-offload.h   |  2 +-
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index 46e7b6db4a1e..48d814664b3d 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,6 +176,7 @@ 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 = this_cpu_ptr(offload->skb_irq_queue);
 	unsigned int i;
 	int received = 0;
 
@@ -190,7 +192,7 @@ 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++;
 	}
@@ -201,6 +203,7 @@ 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 = this_cpu_ptr(offload->skb_irq_queue);
 	struct sk_buff *skb;
 	int received = 0;
 
@@ -211,7 +214,7 @@ 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++;
 	}
 
@@ -222,6 +225,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 = this_cpu_ptr(offload->skb_irq_queue);
 	struct can_rx_offload_cb *cb;
 
 	if (skb_queue_len(&offload->skb_queue) >
@@ -233,7 +237,7 @@ 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,
+	__skb_queue_add_sort(irq_queue, skb,
 			     can_rx_offload_compare);
 
 	return 0;
@@ -268,13 +272,15 @@ 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 = this_cpu_ptr(offload->skb_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);
+	__skb_queue_tail(irq_queue, skb);
 
 	return 0;
 }
@@ -307,14 +313,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 +337,21 @@ 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;
 
 	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 +366,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 +441,14 @@ 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);
+
+	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

Re: [PATCH] can: rx-offload: make skb_irq_queue per-CPU

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Per-CPU splicing destroys strict CAN frame timestamp ordering, causing out-of-order delivery.
- [High] Unsafe use of this_cpu_ptr() in preemptible threaded IRQ context triggers smp_processor_id() warnings.
- [High] Crash in can_rx_offload_del() due to unchecked dynamic per-CPU allocation failure.
- [High] Lockless cross-CPU list manipulation in can_rx_offload_threaded_irq_finish() races with hard IRQ handlers.
- [Medium] Memory leak in at91_can driver due to missing can_rx_offload_del() call.
--

commit 65bff5b50e9fe16e30c0b8850186d29135b0b545
Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
Subject: can: rx-offload: make skb_irq_queue per-CPU

This patch changes the skb_irq_queue from a single lockless list to a
per-CPU list to avoid corruption when multiple mailbox IRQ lines are affined
to different CPUs. The queues are now enqueued via this_cpu_ptr() and
spliced together in the IRQ finish handlers.

> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> index 46e7b6db4a1ef..48d814664b3d1 100644
> --- a/drivers/net/can/dev/rx-offload.c
> +++ b/drivers/net/can/dev/rx-offload.c
[ ... ]
> @@ -222,6 +225,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 = this_cpu_ptr(offload->skb_irq_queue);

[Severity: High]
Is it safe to use this_cpu_ptr() here?

If a driver utilizes threaded IRQs (such as mcp251xfd), it can invoke offload
enqueue helpers from a preemptible context. Wouldn't obtaining a per-CPU
pointer without first disabling preemption trigger a
BUG: using smp_processor_id() in preemptible warning?

Could this also cause lockless list corruption if the kernel thread migrates
to another CPU mid-enqueue?

[ ... ]
> @@ -268,13 +272,15 @@ 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 = this_cpu_ptr(offload->skb_irq_queue);

[Severity: High]
This appears to be another helper used by drivers from potentially preemptible
contexts, and it might have the same this_cpu_ptr() issue as above.

[ ... ]
> @@ -330,15 +337,21 @@ 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;
>  
>  	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 this per-CPU splicing destroy strict CAN frame timestamp ordering?

The commit message states "Cross-line frames are now sorted by timestamp only
within a CPU's queue and appended across CPUs on splice". By blindly
concatenating per-CPU queues in numerical CPU order without merging by
timestamp, doesn't this result in out-of-order delivery of CAN frames to the
network stack for devices receiving frames concurrently on different CPUs?

Could this break chronological CAN bus protocol guarantees?

[Severity: High]
Additionally, can this cross-CPU list manipulation race with hard IRQ handlers?

Here, the threaded IRQ handler iterates over all CPUs and accesses another
CPU's lockless list (irq_queue) without cross-CPU synchronization. If a device
has multiple IRQ lines affined to different CPUs, a hard IRQ handler on
another CPU could concurrently locklessly enqueue into its list via
__skb_queue_add_sort(). 

Couldn't this concurrent modification cause severe list corruption?

[ ... ]
> @@ -353,13 +366,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;

[Severity: Medium]
Will this new dynamic allocation cause memory leaks in drivers that don't call
can_rx_offload_del()?

For example, the at91_can driver calls can_rx_offload_add_timestamp() during
probe but misses the matching can_rx_offload_del() call in its teardown path
(at91_can_remove). Before this patch, the omission was benign, but won't this
now unconditionally leak per-CPU memory upon device unbind, module removal, or
probe failure?

[ ... ]
> @@ -420,8 +441,14 @@ 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);
> +
> +	for_each_possible_cpu(cpu)
> +		__skb_queue_purge(per_cpu_ptr(offload->skb_irq_queue, cpu));

[Severity: High]
Can this cause a kernel panic if the dynamic allocation fails?

If alloc_percpu() fails in can_rx_offload_init_queue(), offload->skb_irq_queue
will be NULL. If a driver ignores the return value (like gs_usb), or during a
probe failure rollback, can_rx_offload_del() will unconditionally call
per_cpu_ptr(NULL, cpu). 

Wouldn't passing this pointer to __skb_queue_purge() blindly overwrite
unrelated per-CPU memory?

> +
> +	free_percpu(offload->skb_irq_queue);
>  }

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

Re: [PATCH] can: rx-offload: make skb_irq_queue per-CPU

[Ciprian Marian Costea]

On 8/31/2026 8:58 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Per-CPU splicing destroys strict CAN frame timestamp ordering, causing out-of-order delivery.
> - [High] Unsafe use of this_cpu_ptr() in preemptible threaded IRQ context triggers smp_processor_id() warnings.
> - [High] Crash in can_rx_offload_del() due to unchecked dynamic per-CPU allocation failure.
> - [High] Lockless cross-CPU list manipulation in can_rx_offload_threaded_irq_finish() races with hard IRQ handlers.
> - [Medium] Memory leak in at91_can driver due to missing can_rx_offload_del() call.
> --
> 
> commit 65bff5b50e9fe16e30c0b8850186d29135b0b545
> Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> Subject: can: rx-offload: make skb_irq_queue per-CPU
> 
> This patch changes the skb_irq_queue from a single lockless list to a
> per-CPU list to avoid corruption when multiple mailbox IRQ lines are affined
> to different CPUs. The queues are now enqueued via this_cpu_ptr() and
> spliced together in the IRQ finish handlers.
> 
>> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
>> index 46e7b6db4a1ef..48d814664b3d1 100644
>> --- a/drivers/net/can/dev/rx-offload.c
>> +++ b/drivers/net/can/dev/rx-offload.c
> [ ... ]
>> @@ -222,6 +225,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 = this_cpu_ptr(offload->skb_irq_queue);
> 
> [Severity: High]
> Is it safe to use this_cpu_ptr() here?
> 
> If a driver utilizes threaded IRQs (such as mcp251xfd), it can invoke offload
> enqueue helpers from a preemptible context. Wouldn't obtaining a per-CPU
> pointer without first disabling preemption trigger a
> BUG: using smp_processor_id() in preemptible warning?
> 
> Could this also cause lockless list corruption if the kernel thread migrates
> to another CPU mid-enqueue?

Good finding. Will fix in V2 by converting all enqueue helpers to use
get_cpu_ptr()/put_cpu_ptr().

> 
> [ ... ]
>> @@ -268,13 +272,15 @@ 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 = this_cpu_ptr(offload->skb_irq_queue);
> 
> [Severity: High]
> This appears to be another helper used by drivers from potentially preemptible
> contexts, and it might have the same this_cpu_ptr() issue as above.

Will fix in V2 by converting all enqueue helpers to use
get_cpu_ptr()/put_cpu_ptr().

> 
> [ ... ]
>> @@ -330,15 +337,21 @@ 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;
>>   
>>   	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 this per-CPU splicing destroy strict CAN frame timestamp ordering?
> 
> The commit message states "Cross-line frames are now sorted by timestamp only
> within a CPU's queue and appended across CPUs on splice". By blindly
> concatenating per-CPU queues in numerical CPU order without merging by
> timestamp, doesn't this result in out-of-order delivery of CAN frames to the
> network stack for devices receiving frames concurrently on different CPUs?
> 
> Could this break chronological CAN bus protocol guarantees?

AFAIK, SocketCAN does not guarantee chronological order. I could do a
cross-CPU merge sort but I do not find it necessary at this point.

> 
> [Severity: High]
> Additionally, can this cross-CPU list manipulation race with hard IRQ handlers?
> 
> Here, the threaded IRQ handler iterates over all CPUs and accesses another
> CPU's lockless list (irq_queue) without cross-CPU synchronization. If a device
> has multiple IRQ lines affined to different CPUs, a hard IRQ handler on
> another CPU could concurrently locklessly enqueue into its list via
> __skb_queue_add_sort().
> 
> Couldn't this concurrent modification cause severe list corruption?
> 

Currently, drivers which use 'can_rx_offload_threaded_irq_finish' are
all single irq. Multi irq drivers use the non-threaded finish variant.
Will add a comment documenting this in V2.

> [ ... ]
>> @@ -353,13 +366,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;
> 
> [Severity: Medium]
> Will this new dynamic allocation cause memory leaks in drivers that don't call
> can_rx_offload_del()?
> 
> For example, the at91_can driver calls can_rx_offload_add_timestamp() during
> probe but misses the matching can_rx_offload_del() call in its teardown path
> (at91_can_remove). Before this patch, the omission was benign, but won't this
> now unconditionally leak per-CPU memory upon device unbind, module removal, or
> probe failure?

Will fix in 'at91_can_remove' in a new patch in V2 in this
series.

> 
> [ ... ]
>> @@ -420,8 +441,14 @@ 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);
>> +
>> +	for_each_possible_cpu(cpu)
>> +		__skb_queue_purge(per_cpu_ptr(offload->skb_irq_queue, cpu));
> 
> [Severity: High]
> Can this cause a kernel panic if the dynamic allocation fails?
> 
> If alloc_percpu() fails in can_rx_offload_init_queue(), offload->skb_irq_queue
> will be NULL. If a driver ignores the return value (like gs_usb), or during a
> probe failure rollback, can_rx_offload_del() will unconditionally call
> per_cpu_ptr(NULL, cpu).
> 
> Wouldn't passing this pointer to __skb_queue_purge() blindly overwrite
> unrelated per-CPU memory?
> 

Good finding. Will fix in V2.

Ciprian

>> +
>> +	free_percpu(offload->skb_irq_queue);
>>   }
>