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

[sashiko-bot@kernel.org]

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