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

[Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>]

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);
>>   }
>