Re: [PATCH v5 1/3] can: rx-offload: make skb_irq_queue per-CPU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Scheduling while atomic BUG on PREEMPT_RT and high preemption latency due to wrapping hardware drain loop in get_cpu_ptr().
- [High] Lockless list corruption and smp_processor_id() BUG on PREEMPT_RT due to preemptible use of this_cpu_ptr() and splice in can_rx_offload_irq_finish().
--

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

can: rx-offload: make skb_irq_queue per-CPU

This commit makes skb_irq_queue per-CPU to prevent concurrent list
corruption when multiple IRQ lines are affined to different CPUs. It
uses get_cpu_ptr() during enqueue to disable preemption, and splices
the queues 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..7616a16d90497 100644
> --- a/drivers/net/can/dev/rx-offload.c
> +++ b/drivers/net/can/dev/rx-offload.c
[ ... ]
> @@ -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;
>  

[Severity: High]
Does wrapping this entire hardware drain loop in get_cpu_ptr() cause a
"scheduling while atomic" BUG on PREEMPT_RT?

Inside the loop in drivers/net/can/dev/rx-offload.c,
can_rx_offload_offload_one() allocates memory. On PREEMPT_RT, GFP_ATOMIC
allocations acquire a local_lock, which maps to an rt_mutex and can sleep.
Acquiring a sleeping lock while preemption is explicitly disabled by
get_cpu_ptr() triggers the BUG splat, and can also cause significant preemption
latency spikes on non-RT systems.

Could get_cpu_ptr() be pushed down into the loop to strictly guard only the
lockless __skb_queue_add_sort() operation, similar to how it was correctly
done in can_rx_offload_queue_timestamp()?

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

[Severity: High]
Does this unbounded read loop have the same "scheduling while atomic" issue
on PREEMPT_RT as in can_rx_offload_irq_offload_timestamp()?

By calling get_cpu_ptr() outside the while loop, preemption is disabled across
all memory allocations in can_rx_offload_offload_one() here as well.

[ ... ]
> @@ -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);

[Severity: High]
Does calling this_cpu_ptr() here in a preemptible context trigger a
"using smp_processor_id() in preemptible" BUG on PREEMPT_RT?

On PREEMPT_RT, standard hardware IRQ handlers like flexcan_irq execute as
preemptible IRQ threads. Because preemption is not disabled here, calling
this_cpu_ptr() is unsafe.

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

[Severity: High]
Can another IRQ thread preempt this list splice and corrupt the list?

Since preemption is not disabled in this function (and spin_lock_irqsave
only disables migration on RT, not preemption), a higher-priority IRQ
thread on a multi-IRQ controller (like FlexCAN on S32G2) could preempt
the lower-priority one during this skb_queue_splice_tail_init() splice.

If the preempting thread then calls can_rx_offload_irq_offload_timestamp(),
it will explicitly disable preemption and push to the exact same CPU's
irq_queue, concurrently modifying the lockless list.

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