Re: [PATCH RESEND v4 1/8] can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The split IRQ handlers break the `FLEXCAN_QUIRK_BROKEN_PERR_STATE` workaround on multi-IRQ platforms, causing silent error state transitions to go undetected.

Pre-existing issues:
- [Critical] Concurrent execution of split IRQ handlers on multi-IRQ platforms causes unsynchronized lockless list corruption in `skb_irq_queue`.
--

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

can: flexcan: use dedicated IRQ handlers for multi-IRQ platforms
    
On platforms with multiple IRQ lines (S32G2, MCF5441X), all lines are
registered to the same flexcan_irq() handler. Since these are distinct IRQ
numbers, they can be dispatched concurrently on different CPUs. Both
instances then read the same iflag and ESR registers unconditionally,
leading to duplicate frame processing.

> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f5d22c61503fa..f73ff442d5304 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c

[ ... ]

> +/* Mailbox IRQ handler for multi-IRQ platforms */
> +static irqreturn_t flexcan_irq_mb(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	irqreturn_t handled;
> +
> +	handled = flexcan_do_mb(dev);

[Severity: High]
Does this change break the FLEXCAN_QUIRK_BROKEN_PERR_STATE workaround on
multi-IRQ platforms?

Before this patch, the single monolithic flexcan_irq() mitigated the hardware
erratum by unconditionally polling the error state via flexcan_do_state() on
every interrupt. By leveraging frequent mailbox TX/RX events, it could detect
silent state transitions.

With dedicated handlers, flexcan_irq_mb() now exclusively processes mailbox
events and skips calling flexcan_do_state(). Since the hardware bug prevents
the boff interrupt from firing during state transitions, will the device now
silently enter Error Passive or Bus Off without the software noticing?

> +
> +	if (handled)
> +		can_rx_offload_irq_finish(&priv->offload);
> +
> +	return handled;
> +}
> +
> +/* Bus error IRQ handler for multi-IRQ platforms */
> +static irqreturn_t flexcan_irq_berr(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	irqreturn_t handled;
> +
> +	handled = flexcan_do_berr(dev);

[Severity: Critical]
This is a pre-existing issue, but does concurrent execution of these split IRQ
handlers on multi-IRQ platforms cause unsynchronized lockless list corruption?

On platforms with multiple IRQ lines, the distinct hardware interrupts (such
as MB and BERR) can be dispatched concurrently on different CPUs. The handlers
flexcan_irq_mb() and flexcan_irq_berr() concurrently invoke can_rx_offload
functions. For example, flexcan_irq_berr() eventually calls
can_rx_offload_queue_timestamp() via flexcan_irq_bus_err().

These offload functions call __skb_queue_add_sort(&offload->skb_irq_queue, ...),
which performs unsynchronized doubly-linked list pointer manipulation. Since
there are no locks protecting skb_irq_queue, concurrent enqueues from
different CPUs can corrupt the list pointers, resulting in use-after-free
or kernel panics.

While this patch explicitly tries to fix duplicate processing caused by
concurrency, the concurrent pushes to the lockless skb_irq_queue seem to
remain unhandled.

> +
> +	if (handled)
> +		can_rx_offload_irq_finish(&priv->offload);
> +
> +	return handled;
> +}

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