Re: [PATCH v5 1/3] can: rx-offload: make skb_irq_queue per-CPU
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-07 11:12:28
Also in:
imx
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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()?
quoted hunk ↗ jump to hunk
- __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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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