Skip to contentSkip to conversation

hwmon: (gpio-fan) return IRQ_HANDLED from the shared alarm IRQ handler

PATCH Superseded Cong Nguyen 3 participants 5 messages Sep 1 – Sep 14 Hardware monitoring

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Cong Nguyen

author
#0 lore raw

PATCH 1/1 hwmon: (gpio-fan) return IRQ_HANDLED from the shared alarm IRQ handler

fan_alarm_irq_handler() is IRQF_SHARED but unconditionally returns IRQ_NONE, even though it always schedules work in response. Enough "unhandled" reports on a shared line trips the kernel's spurious- interrupt detection and disables it, taking every device sharing it down too.

Return IRQ_HANDLED, matching what the handler actually does.

Fixes d6fe1360f42e ("hwmon: add generic GPIO fan driver")
Reported-by Sashiko AI review <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Assisted-by Claude:claude-opus-4
Signed-off-by Cong Nguyen <[email protected]>
---
 drivers/hwmon/gpio-fan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/gpio-fan.c +1 -1
index 084828e1e281..a55a7ea07fc0 100644
@@ -67,7 +67,7 @@ static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id)
 
 	schedule_work(&fan_data->alarm_work);
 
-	return IRQ_NONE;
+	return IRQ_HANDLED;
 }
 
 static ssize_t fan1_alarm_show(struct device *dev,
Signature · 1 line
2.25.1
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

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

New issues:
- [High] The interrupt handler unconditionally returns IRQ_HANDLED while registered as IRQF_SHARED, neutralizing spurious interrupt detection and risking a CPU lockup.

Pre-existing issues:
- [High] Missing cancellation of alarm_work during driver unbind leads to a Use-After-Free vulnerability. --

commit 2cca5798d0ba1012484e76e484ce607c8ed4c2f0
Author: Cong Nguyen <[email protected]>

hwmon: (gpio-fan) return IRQ_HANDLED from the shared alarm IRQ handler

This commit changes the interrupt handler for the GPIO fan alarm to return IRQ_HANDLED instead of IRQ_NONE to prevent the spurious interrupt detector from disabling the shared line.

Link https://lore.kernel.org/r/[email protected] open here

drivers/hwmon/gpio-fan.c static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id) · line 68

Quoted 7 lines
drivers/hwmon/gpio-fan.c +0 -0
index 084828e1e2817..a55a7ea07fc01 100644
@@ -67,7 +67,7 @@ static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id)
 
 	schedule_work(&fan_data->alarm_work);

[Severity: High]
This is a pre-existing issue, but does this workqueue lack a cancellation mechanism during driver unbind? The alarm_work is scheduled here and executed asynchronously in fan_alarm_notify(), but there doesn't appear to be a registered devm cleanup action to ensure cancel_work_sync() is called during teardown.

If the device is unbound via sysfs while an alarm condition exists, devres first frees the IRQ line and then the fan_data structure without waiting for pending system workqueues. Could this result in a use-after-free vulnerability when the delayed work executes and dereferences the freed fan_data memory in fan_alarm_notify()?

drivers/hwmon/gpio-fan.c static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id) · line 70

Quoted 3 lines
drivers/hwmon/gpio-fan.c +1 -1
-	return IRQ_NONE;
+	return IRQ_HANDLED;

[Severity: High]
Does this code unconditionally claim every interrupt on the shared line?

Since this handler is registered with IRQF_SHARED, always returning IRQ_HANDLED could mask spurious interrupts from other devices sharing the line. This prevents the core IRQ subsystem from detecting and disabling a faulty line, which might lead to an unmitigated interrupt storm and a CPU lockup.

Should the handler verify if the interrupt was actually generated by the GPIO fan alarm hardware before returning IRQ_HANDLED?

Quoted 1 line
drivers/hwmon/gpio-fan.c +0 -0
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
maintainer
#2 lore raw
Quoted 5 lines · On 9/1/26 09:09, [email protected] wrote:

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

New issues:
- [High] The interrupt handler unconditionally returns IRQ_HANDLED while registered as IRQF_SHARED, neutralizing spurious interrupt detection and risking a CPU lockup.

As I suspected. This interrupt must not be configured as IRQF_SHARED.

Thanks,
Guenter

author
#3 lore raw
Quoted 9 lines · Guenter Roeck
Quoted 5 lines · On 9/1/26 09:09, [email protected] wrote:

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

New issues:
- [High] The interrupt handler unconditionally returns IRQ_HANDLED while registered as IRQF_SHARED, neutralizing spurious interrupt detection and risking a CPU lockup.

As I suspected. This interrupt must not be configured as IRQF_SHARED.

Agreed, sent as v2 -- drops IRQF_SHARED too.

thanks,
Cong

Quoted 4 lines

Thanks,
Guenter

maintainer
#4 lore raw
In reply to Cong Nguyen
Quoted 22 lines · Cong Nguyen

fan_alarm_irq_handler() is IRQF_SHARED but unconditionally returns IRQ_NONE, even though it always schedules work in response. Enough "unhandled" reports on a shared line trips the kernel's spurious- interrupt detection and disables it, taking every device sharing it down too.

Return IRQ_HANDLED, matching what the handler actually does.

Fixes d6fe1360f42e ("hwmon: add generic GPIO fan driver")
Reported-by Sashiko AI review <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Assisted-by Claude:claude-opus-4
Signed-off-by Cong Nguyen <[email protected]>
---
  drivers/hwmon/gpio-fan.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/gpio-fan.c +0 -0
index 084828e1e281..a55a7ea07fc0 100644

drivers/hwmon/gpio-fan.c static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id) · line 72

Quoted 7 lines
drivers/hwmon/gpio-fan.c +1 -1
@@ -67,7 +67,7 @@ static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id)
 
  	schedule_work(&fan_data->alarm_work);
 
-	return IRQ_NONE;
+	return IRQ_HANDLED;
  }

That makes me wonder if IRQF_SHARED is appropriate for this driver's interrupt handler. I think that should be dropped as well.

Thanks,
Guenter

Quoted 1 line
drivers/hwmon/gpio-fan.c +0 -0
  static ssize_t fan1_alarm_show(struct device *dev,