Re: [PATCH v7 4/6] dt-bindings: can: fsl,flexcan: add NXP S32N79 SoC support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent execution of split IRQ handlers corrupts rx-offload skb_irq_queue and CAN state.
--

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

dt-bindings: can: fsl,flexcan: add NXP S32N79 SoC support

Add NXP S32N79 SoC compatible string and interrupt properties.
On S32N79, FlexCAN IP is integrated with two interrupt lines:
one for the mailbox interrupts (0-127) and one for signaling
bus errors and device state changes.

> diff --git a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
> index f81d56f7c12a5..d098a44c2b9c2 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
> +++ b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml

[ ... ]

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: nxp,s32n79-flexcan
> +    then:
> +      properties:
> +        interrupts:
> +          items:
> +            - description: Message Buffer interrupt for mailboxes 0-127
> +            - description: Bus Error and Device state change interrupt

[Severity: High]
This is a pre-existing issue, but this multi-IRQ configuration exposes a
concurrent execution bug in the split IRQ handlers.

When multiple interrupt events (e.g., mailbox processing and bus error) fire
simultaneously, the handlers mapped to different IRQ numbers on multi-IRQ
platforms (like S32G2 or S32N79) can be dispatched concurrently on different
CPU cores.

For example, in drivers/net/can/flexcan/flexcan-core.c, flexcan_irq_mb()
calls flexcan_do_mb():

	handled = flexcan_do_mb(dev, mb_mask);

	if (handled)
		can_rx_offload_irq_finish(&priv->offload);

This can run concurrently with the state change interrupt in flexcan_do_state():

	/* state change interrupt or broken error state quirk fix is enabled */
	if ((reg_esr & FLEXCAN_ESR_ERR_STATE) ||
	    (priv->devtype_data.quirks &
	    (FLEXCAN_QUIRK_BROKEN_WERR_STATE | FLEXCAN_QUIRK_BROKEN_PERR_STATE)))
		flexcan_irq_state(dev, reg_esr);

These concurrent handlers invoke can_rx_offload_irq_offload_timestamp() and
can_rx_offload_queue_timestamp(), which use the lockless
__skb_queue_add_sort() API to insert SKBs into offload->skb_irq_queue.

Since these list insertion APIs omit locking (relying on a single-IRQ
execution context), can this concurrent execution corrupt the doubly-linked
list pointers and lead to a kernel panic during normal CAN traffic?

Additionally, with the FLEXCAN_QUIRK_BROKEN_PERR_STATE quirk, both
flexcan_irq_berr() and flexcan_irq_boff() can concurrently execute
flexcan_do_state(), causing data races in can_change_state().

This issue was introduced by the earlier commit "can: flexcan: use dedicated
IRQ handlers for multi-IRQ platforms" in this patchset, and it was not
resolved by the end of the series. Should locking be introduced to protect
the CAN state and offload queues?

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