Re: [PATCH net v4 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing IRQ teardown in `oa_tc6_exit()` allows the threaded IRQ to run concurrently with or after driver teardown, causing Use-After-Free of SKBs.
- [High] Unmasking a level-triggered interrupt without clearing the hardware condition causes an interrupt storm on SPI/protocol errors.
--

commit 5c9cbbecd5c1eb1d5efb41b338ac224d42d08b34
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>
net: ethernet: oa_tc6: Interrupt is active low, level triggered.

This patch modifies the interrupt handling mechanism for the oa_tc6 driver.
It replaces the kthread-based SPI transfer approach with a threaded IRQ, and
configures the interrupt trigger type from edge-falling to level-low.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 91a906a7918a2..e2c0843207f24 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -1105,29 +1104,18 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
>  	return 0;
>  }
>  
> -static int oa_tc6_spi_thread_handler(void *data)
> +static irqreturn_t oa_tc6_macphy_threaded_irq(int irq, void *data)
>  {
>  	struct oa_tc6 *tc6 = data;
> -	int ret;
> -
> -	while (likely(!kthread_should_stop())) {
> -		/* This kthread will be waken up if there is a tx skb or mac-phy
> -		 * interrupt to perform spi transfer with tx chunks.
> -		 */
> -		wait_event_interruptible(tc6->spi_wq, tc6->int_flag ||
> -					 (tc6->waiting_tx_skb &&
> -					 tc6->tx_credits) ||
> -					 kthread_should_stop());
> -
> -		if (kthread_should_stop())
> -			break;
>  
> +	while (tc6->int_flag || (tc6->waiting_tx_skb && tc6->tx_credits)) {
>  		ret = oa_tc6_try_spi_transfer(tc6);
>  		if (ret)
> -			return ret;
> +			break;
>  	}
>  
> -	return 0;
> +	return IRQ_HANDLED;
>  }

[Severity: High]
If oa_tc6_try_spi_transfer() fails (e.g., due to an SPI transfer failure)
and returns an error, the loop breaks and returns IRQ_HANDLED.

Since the IRQ is now configured as level-triggered, returning IRQ_HANDLED
will cause the genirq core to unmask the interrupt. If the hardware condition
was not successfully cleared due to the transfer failure, won't this result
in an immediate interrupt storm that locks up the CPU?

Should the IRQ be explicitly disabled (e.g., via disable_irq_nosync()) when
aborting on an unrecoverable error?

[ ... ]

> @@ -1357,7 +1331,6 @@ EXPORT_SYMBOL_GPL(oa_tc6_init);
>  void oa_tc6_exit(struct oa_tc6 *tc6)
>  {
>  	oa_tc6_phy_exit(tc6);
> -	kthread_stop(tc6->spi_thread);
>  	dev_kfree_skb_any(tc6->ongoing_tx_skb);
>  	dev_kfree_skb_any(tc6->waiting_tx_skb);
>  	dev_kfree_skb_any(tc6->rx_skb);
>  }

[Severity: High]
Does this code introduce a use-after-free window?

The patch removes kthread_stop(), but it doesn't explicitly disable or free
the newly added threaded IRQ here. Since the IRQ was requested with
devm_request_threaded_irq(), it will remain fully active until the devres
cleanup phase, which occurs after the driver's remove path finishes.

If the threaded IRQ runs concurrently with or after oa_tc6_exit(), can it
access tc6->ongoing_tx_skb, tc6->waiting_tx_skb, or tc6->rx_skb while they
are being unconditionally freed by dev_kfree_skb_any()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-level-trigger-v4-0-6f389abdd192@onsemi.com?part=1