Re: [PATCH] can: sja1000: drop RX frame when skb allocation fails

[luyun <luyun_611@163.com>]

在 2026/9/3 18:04, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] In sja1000_err(), an SKB allocation failure returns -ENOMEM, discarding IRQ_WAKE_THREAD requests and causing the interrupt handler to erroneously return IRQ_NONE.
> --
>
> commit 6507d559134b8295113fac43d6ce819e79885772
> Author: Yun Lu <luyun@kylinos.cn>
>
> can: sja1000: drop RX frame when skb allocation fails
>
> This patch prevents a lockup in sja1000_rx() by dropping the frame and
> releasing the receive buffer if SKB allocation fails.
>
>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
>> index 3cdb583ee3e50..da107feb22af7 100644
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -348,8 +348,12 @@ static void sja1000_rx(struct net_device *dev)
>>   
>>   	/* create zero'ed CAN frame buffer */
>>   	skb = alloc_can_skb(dev, &cf);
>> -	if (skb == NULL)
>> +	if (!skb) {
>> +		/* drop the frame to guarantee forward progress of the ISR */
>> +		sja1000_write_cmdreg(priv, CMD_RRB);
>> +		stats->rx_dropped++;
>>   		return;
>> +	}
>>   
>>   	fi = priv->read_reg(priv, SJA1000_FI);
> [Severity: High]
> This is a pre-existing issue, but while looking at SKB allocation failures
> in the interrupt handler, does a similar allocation failure in
> sja1000_err() lead to the IRQ line being permanently disabled?
>
> Under memory pressure, if the SKB allocation fails in sja1000_err():
>
> drivers/net/can/sja1000/sja1000.c:sja1000_err() {
>      ...
> 	if (!skb)
> 		return -ENOMEM;
>
> 	netif_rx(skb);
>
> 	return ret;
> }
>
> This unconditionally returns -ENOMEM, which discards any IRQ_WAKE_THREAD
> requests needed for overrun recovery like SJA1000_QUIRK_RESET_ON_OVERRUN.
>
> This error propagates to sja1000_interrupt(), causing the loop to break
> before incrementing n:
>
> drivers/net/can/sja1000/sja1000.c:sja1000_interrupt() {
>      ...
> 			err = sja1000_err(dev, isrc, status);
> 			if (err == IRQ_WAKE_THREAD)
> 				ret = err;
> 			if (err)
> 				break;
> 		}
> 		n++;
>      ...
> }
>
> If this happens on the first loop iteration where n is 0, the handler
> returns IRQ_NONE despite having successfully acknowledged the interrupt in
> hardware:
>
> drivers/net/can/sja1000/sja1000.c:sja1000_interrupt() {
>      ...
> out:
> 	if (!ret)
> 		ret = (n) ? IRQ_HANDLED : IRQ_NONE;
>      ...
> }
>
> Could repeated IRQ_NONE returns under sustained memory pressure cause the
> core IRQ subsystem to permanently disable the device's IRQ line?

Thanks for pointing this out.

The IRQ_WAKE_THREAD result is indeed lost when alloc_can_err_skb()
fails, because sja1000_err() returns -ENOMEM instead of the value stored
in ret. This can prevent the threaded reset handler from running on
controllers with SJA1000_QUIRK_RESET_ON_OVERRUN. It may also cause the
top-level handler to return IRQ_NONE when the failure occurs during the
first loop iteration.

But this is separate from the issue addressed by the current patch. The
current patch fixes the receive path in sja1000_rx(), where an allocation
failure leaves the RX buffer unreleased and prevents the ISR from making
progress. The issue reported here concerns error interrupt handling in
sja1000_err() and the propagation of its IRQ return value.

Would it be better to address this in a separate patch?  Since the two 
issues
affect different paths and have different failure modes, I think this should
be addressed in a separate patch.

Please let me know if you would prefer it to be included in a v2 series 
instead.

---

Thanks.

Yun Lu