Re: [PATCH v5 5/8] can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Concurrent lockless modification of `skb_irq_queue` due to multiple independent IRQ handlers.
--

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

can: flexcan: add FLEXCAN_QUIRK_IRQ_BERR quirk

This patch introduces the FLEXCAN_QUIRK_IRQ_BERR quirk for hardware that
has a dedicated interrupt line for bus errors and state changes. It adds
the flexcan_irq_esr() handler and updates IRQ enabling/disabling to
support the NXP S32N79 SoC.

> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -1300,6 +1300,22 @@ static irqreturn_t flexcan_irq_boff(int irq, void *dev_id)
>  	return handled;
>  }
>  
> +/* Combined bus error and state change IRQ handler */
> +static irqreturn_t flexcan_irq_esr(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	irqreturn_t handled;
> +
> +	handled = flexcan_do_state(dev);
> +	handled |= flexcan_do_berr(dev);
> +
> +	if (handled)
> +		can_rx_offload_irq_finish(&priv->offload);
> +
> +	return handled;
> +}
> +

[ ... ]

> @@ -1902,6 +1919,13 @@ static int flexcan_open(struct net_device *dev)
>  			goto out_free_irq_boff;
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_IRQ_BERR) {
> +		err = request_irq(priv->irq_err,
> +				  flexcan_irq_esr, IRQF_SHARED, dev->name, dev);

[Severity: Critical]
This isn't a bug introduced by this patch, but it expands a pre-existing
race condition to the S32N79 SoC by registering another concurrent hardware
IRQ handler.

Does registering multiple independent IRQ lines lead to concurrent lockless
modifications of the CAN rx-offload subsystem's skb_irq_queue?

If flexcan_irq_mb() and flexcan_irq_esr() trigger concurrently on different
CPUs:

CPU1 (flexcan_irq_mb)
flexcan_do_mb()
   can_rx_offload_queue_timestamp()
      __skb_queue_add_sort()  <-- lockless push

CPU2 (flexcan_irq_esr)
flexcan_do_state()
   can_rx_offload_queue_timestamp()
      __skb_queue_add_sort()  <-- concurrent lockless push

Or CPU1 could call can_rx_offload_irq_finish() (which splices the queue)
while CPU2 is pushing a new skb. Can this corrupt the doubly linked
skb_irq_queue pointers and cause kernel panics?

> +		if (err)
> +			goto out_free_irq_boff;
> +	}

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