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

[sashiko-bot@kernel.org]

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