Skip to contentSkip to conversation

iio: trigger: free trigger resource correctly

PATCH Applied Alison Schofield 3 participants 4 messages Jan 20, 2017 – Jan 22, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 iio: trigger: free trigger resource correctly

These stand-alone trigger drivers were using iio_trigger_put() where they should have been using iio_trigger_free(). The iio_trigger_put() adds a module_put which is bad since they never did a module_get.

In the sysfs driver, module_get/put's are used as triggers are added & removed. This extra module_put() occurs on an error path in the probe routine (probably rare).

In the bfin-timer & interrupt trigger drivers, the module resources are not explicitly managed, so it's doing a put on something that was never get'd. It occurs on the probe error path and on the remove path (not so rare).

Tested with the sysfs trigger driver.
The bfin & interrupt drivers were build tested & inspected only.

---
Changes in v2:
Renamed probe jump label to say free instead of put. Added further explanation to commit log.

Note from v1:
Patches to use devm_* funcs are ready to follow this for the interrupt & bfin-timer triggers.

 drivers/iio/trigger/iio-trig-interrupt.c          | 8 ++++----
 drivers/iio/trigger/iio-trig-sysfs.c              | 2 +-
 drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/iio/trigger/iio-trig-interrupt.c +4 -4
index 572bc6f..e18f12b 100644
@@ -58,7 +58,7 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
 	if (!trig_info) {
 		ret = -ENOMEM;
-		goto error_put_trigger;
+		goto error_free_trigger;
 	}
 	iio_trigger_set_drvdata(trig, trig_info);
 	trig_info->irq = irq;
@@ -83,8 +83,8 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 	free_irq(irq, trig);
 error_free_trig_info:
 	kfree(trig_info);
-error_put_trigger:
-	iio_trigger_put(trig);
+error_free_trigger:
+	iio_trigger_free(trig);
 error_ret:
 	return ret;
 }
@@ -99,7 +99,7 @@ static int iio_interrupt_trigger_remove(struct platform_device *pdev)
 	iio_trigger_unregister(trig);
 	free_irq(trig_info->irq, trig);
 	kfree(trig_info);
-	iio_trigger_put(trig);
+	iio_trigger_free(trig);
 
 	return 0;
 }
drivers/iio/trigger/iio-trig-sysfs.c +1 -1
index 3dfab2b..202e8b8 100644
@@ -174,7 +174,7 @@ static int iio_sysfs_trigger_probe(int id)
 	return 0;
 
 out2:
-	iio_trigger_put(t->trig);
+	iio_trigger_free(t->trig);
 free_t:
 	kfree(t);
 out1:
drivers/staging/iio/trigger/iio-trig-bfin-timer.c +2 -2
index 9658f20..4e0b4ee 100644
@@ -260,7 +260,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
 out1:
 	iio_trigger_unregister(st->trig);
 out:
-	iio_trigger_put(st->trig);
+	iio_trigger_free(st->trig);
 	return ret;
 }
 
@@ -273,7 +273,7 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev)
 		peripheral_free(st->t->pin);
 	free_irq(st->irq, st);
 	iio_trigger_unregister(st->trig);
-	iio_trigger_put(st->trig);
+	iio_trigger_free(st->trig);
 
 	return 0;
 }
Signature · 1 line
2.1.4
maintainer
#1 lore raw
Quoted 14 lines · Alison Schofield

These stand-alone trigger drivers were using iio_trigger_put() where they should have been using iio_trigger_free(). The iio_trigger_put() adds a module_put which is bad since they never did a module_get.

In the sysfs driver, module_get/put's are used as triggers are added & removed. This extra module_put() occurs on an error path in the probe routine (probably rare).

In the bfin-timer & interrupt trigger drivers, the module resources are not explicitly managed, so it's doing a put on something that was never get'd. It occurs on the probe error path and on the remove path (not so rare).

Quoted 4 lines

Tested with the sysfs trigger driver.
The bfin & interrupt drivers were build tested & inspected only.

This is certainly more consistent.
Lars, could you sanity check as well given the bfin timer is in here.

Thanks,

Jonathan

Quoted 84 lines

---
Changes in v2:
Renamed probe jump label to say free instead of put. Added further explanation to commit log.

Note from v1:
Patches to use devm_* funcs are ready to follow this for the interrupt & bfin-timer triggers.

 drivers/iio/trigger/iio-trig-interrupt.c          | 8 ++++----
 drivers/iio/trigger/iio-trig-sysfs.c              | 2 +-
 drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/iio/trigger/iio-trig-interrupt.c +4 -4
index 572bc6f..e18f12b 100644
@@ -58,7 +58,7 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
 	if (!trig_info) {
 		ret = -ENOMEM;
-		goto error_put_trigger;
+		goto error_free_trigger;
 	}
 	iio_trigger_set_drvdata(trig, trig_info);
 	trig_info->irq = irq;
@@ -83,8 +83,8 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev)
 	free_irq(irq, trig);
 error_free_trig_info:
 	kfree(trig_info);
-error_put_trigger:
-	iio_trigger_put(trig);
+error_free_trigger:
+	iio_trigger_free(trig);
 error_ret:
 	return ret;
 }
@@ -99,7 +99,7 @@ static int iio_interrupt_trigger_remove(struct platform_device *pdev)
 	iio_trigger_unregister(trig);
 	free_irq(trig_info->irq, trig);
 	kfree(trig_info);
-	iio_trigger_put(trig);
+	iio_trigger_free(trig);
 
 	return 0;
 }
drivers/iio/trigger/iio-trig-sysfs.c +1 -1
index 3dfab2b..202e8b8 100644
@@ -174,7 +174,7 @@ static int iio_sysfs_trigger_probe(int id)
 	return 0;
 
 out2:
-	iio_trigger_put(t->trig);
+	iio_trigger_free(t->trig);
 free_t:
 	kfree(t);
 out1:
drivers/staging/iio/trigger/iio-trig-bfin-timer.c +2 -2
index 9658f20..4e0b4ee 100644
@@ -260,7 +260,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
 out1:
 	iio_trigger_unregister(st->trig);
 out:
-	iio_trigger_put(st->trig);
+	iio_trigger_free(st->trig);
 	return ret;
 }
 
@@ -273,7 +273,7 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev)
 		peripheral_free(st->t->pin);
 	free_irq(st->irq, st);
 	iio_trigger_unregister(st->trig);
-	iio_trigger_put(st->trig);
+	iio_trigger_free(st->trig);
 
 	return 0;
 }
#2 lore raw
Quoted 15 lines · Jonathan Cameron
Quoted 14 lines · Alison Schofield

These stand-alone trigger drivers were using iio_trigger_put() where they should have been using iio_trigger_free(). The iio_trigger_put() adds a module_put which is bad since they never did a module_get.

In the sysfs driver, module_get/put's are used as triggers are added & removed. This extra module_put() occurs on an error path in the probe routine (probably rare).

In the bfin-timer & interrupt trigger drivers, the module resources are not explicitly managed, so it's doing a put on something that was never get'd. It occurs on the probe error path and on the remove path (not so rare).

Quoted 7 lines
Quoted 4 lines

Tested with the sysfs trigger driver.
The bfin & interrupt drivers were build tested & inspected only.

This is certainly more consistent.
Lars, could you sanity check as well given the bfin timer is in here.

looks good.

maintainer Applied
#3 lore raw
Quoted 19 lines · Lars-Peter Clausen
Quoted 18 lines · Jonathan Cameron
Quoted 17 lines · Alison Schofield

These stand-alone trigger drivers were using iio_trigger_put() where they should have been using iio_trigger_free(). The iio_trigger_put() adds a module_put which is bad since they never did a module_get.

In the sysfs driver, module_get/put's are used as triggers are added & removed. This extra module_put() occurs on an error path in the probe routine (probably rare).

In the bfin-timer & interrupt trigger drivers, the module resources are not explicitly managed, so it's doing a put on something that was never get'd. It occurs on the probe error path and on the remove path (not so rare).

Tested with the sysfs trigger driver.
The bfin & interrupt drivers were build tested & inspected only.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

This is certainly more consistent.
Lars, could you sanity check as well given the bfin timer is in here.

looks good.

Applied to the togreg branch of iio.git.

Thought about sending this as a fix, but as it's been there a long time I've taken the view there is no real rush on this.

Thanks,

Jonathan