* [PATCH] Staging: iio: trigger: Use devm functions
@ 2016-09-18 21:16 Bhumika Goyal
2016-09-19 5:44 ` [Outreachy kernel] " Alison Schofield
0 siblings, 1 reply; 6+ messages in thread
From: Bhumika Goyal @ 2016-09-18 21:16 UTC (permalink / raw)
To: outreachy-kernel, jic23, knaack.h, lars, pmeerw, gregkh; +Cc: Bhumika Goyal
Use managed resource functions devm_iio_trigger_alloc and
devm_request_irq instead of iio_trigger_alloc and request_irq.
Remove corresponding calls to free_irq in the probe and remove
functions. Drop the now unnecessary goto labels and replace various
gotos with direct returns.
request_irq replacement done using coccinelle.
@platform@
identifier p, probefn, removefn;
@@
struct platform_driver p = {
.probe = probefn,
.remove = removefn,
};
@prb@
identifier platform.probefn, pdev;
expression list E;
expression e;
@@
probefn(struct platform_device *pdev, ...) {
<+...
- e = request_irq(E);
+ e = devm_request_irq(&pdev->dev,E);
...
?-free_irq(...);
...+>
}
@rem depends on prb@
identifier platform.removefn;
expression e;
@@
removefn(...) {
<...
- free_irq(...);
...>
}
Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
---
drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
index 38dca69..ea4845c 100644
--- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
+++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
@@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
st->timer_num = ret;
st->t = &iio_bfin_timer_code[st->timer_num];
- st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num);
+ st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d",
+ st->timer_num);
if (!st->trig)
return -ENOMEM;
@@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
if (ret)
goto out;
- ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr,
- 0, st->trig->name, st);
+ ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr,
+ 0, st->trig->name, st);
if (ret) {
dev_err(&pdev->dev,
"request IRQ-%d failed", st->irq);
@@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
ret = peripheral_request(st->t->pin, st->trig->name);
if (ret)
- goto out_free_irq;
+ return ret;
val = (unsigned long long)get_sclk() * pdata->duty_ns;
do_div(val, NSEC_PER_SEC);
@@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, st);
return 0;
-out_free_irq:
- free_irq(st->irq, st);
+
out1:
iio_trigger_unregister(st->trig);
out:
@@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev)
disable_gptimers(st->t->bit);
if (st->output_enable)
peripheral_free(st->t->pin);
- free_irq(st->irq, st);
iio_trigger_unregister(st->trig);
iio_trigger_put(st->trig);
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions 2016-09-18 21:16 [PATCH] Staging: iio: trigger: Use devm functions Bhumika Goyal @ 2016-09-19 5:44 ` Alison Schofield 2016-09-19 5:53 ` Bhumika Goyal 0 siblings, 1 reply; 6+ messages in thread From: Alison Schofield @ 2016-09-19 5:44 UTC (permalink / raw) To: Bhumika Goyal; +Cc: outreachy-kernel, jic23, knaack.h, lars, pmeerw, gregkh On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote: > Use managed resource functions devm_iio_trigger_alloc and > devm_request_irq instead of iio_trigger_alloc and request_irq. > Remove corresponding calls to free_irq in the probe and remove > functions. Drop the now unnecessary goto labels and replace various > gotos with direct returns. > request_irq replacement done using coccinelle. Hi Bhumika, Thanks for the patch. This seems like it's worthy of 2 separate patches. Also, I'm a bit confused with why I don't see an iio_trigger_free that is going away, when you switch to the devm_iio_trigger_alloc functions, but I realize there isn't one there to remove. hmm??? alisons > > @platform@ > identifier p, probefn, removefn; > @@ > struct platform_driver p = { > .probe = probefn, > .remove = removefn, > }; > > @prb@ > identifier platform.probefn, pdev; > expression list E; > expression e; > > @@ > probefn(struct platform_device *pdev, ...) { > <+... > - e = request_irq(E); > + e = devm_request_irq(&pdev->dev,E); > ... > ?-free_irq(...); > ...+> > } > > @rem depends on prb@ > identifier platform.removefn; > expression e; > @@ > removefn(...) { > <... > - free_irq(...); > ...> > } > > Signed-off-by: Bhumika Goyal <bhumirks@gmail.com> > --- > drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c > index 38dca69..ea4845c 100644 > --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c > +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c > @@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > st->timer_num = ret; > st->t = &iio_bfin_timer_code[st->timer_num]; > > - st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num); > + st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d", > + st->timer_num); > if (!st->trig) > return -ENOMEM; > > @@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > if (ret) > goto out; > > - ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr, > - 0, st->trig->name, st); > + ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr, > + 0, st->trig->name, st); > if (ret) { > dev_err(&pdev->dev, > "request IRQ-%d failed", st->irq); > @@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > > ret = peripheral_request(st->t->pin, st->trig->name); > if (ret) > - goto out_free_irq; > + return ret; > > val = (unsigned long long)get_sclk() * pdata->duty_ns; > do_div(val, NSEC_PER_SEC); > @@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, st); > > return 0; > -out_free_irq: > - free_irq(st->irq, st); > + > out1: > iio_trigger_unregister(st->trig); > out: > @@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev) > disable_gptimers(st->t->bit); > if (st->output_enable) > peripheral_free(st->t->pin); > - free_irq(st->irq, st); > iio_trigger_unregister(st->trig); > iio_trigger_put(st->trig); > > -- > 1.9.1 > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1474233410-6932-1-git-send-email-bhumirks%40gmail.com. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions 2016-09-19 5:44 ` [Outreachy kernel] " Alison Schofield @ 2016-09-19 5:53 ` Bhumika Goyal 2016-09-19 9:21 ` Julia Lawall 0 siblings, 1 reply; 6+ messages in thread From: Bhumika Goyal @ 2016-09-19 5:53 UTC (permalink / raw) To: Alison Schofield Cc: outreachy-kernel, jic23, knaack.h, Lars-Peter Clausen, Peter Meerwald-Stadler, gregkh@linuxfoundation.org On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote: > On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote: >> Use managed resource functions devm_iio_trigger_alloc and >> devm_request_irq instead of iio_trigger_alloc and request_irq. >> Remove corresponding calls to free_irq in the probe and remove >> functions. Drop the now unnecessary goto labels and replace various >> gotos with direct returns. >> request_irq replacement done using coccinelle. > > Hi Bhumika, > Thanks for the patch. This seems like it's worthy of 2 separate > patches. > > Also, I'm a bit confused with why I don't see an iio_trigger_free > that is going away, when you switch to the devm_iio_trigger_alloc > functions, but I realize there isn't one there to remove. hmm??? > > alisons > Hey Alison, Ok, I will separate this in two patches. I will do devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as devm_request_irq requires all previous memory allocations to be devms. Regarding iio_trigger_free I didn't find any in either of the two functions(probe and remove). Thanks for the input. Thanks, Bhumika > > >> >> @platform@ >> identifier p, probefn, removefn; >> @@ >> struct platform_driver p = { >> .probe = probefn, >> .remove = removefn, >> }; >> >> @prb@ >> identifier platform.probefn, pdev; >> expression list E; >> expression e; >> >> @@ >> probefn(struct platform_device *pdev, ...) { >> <+... >> - e = request_irq(E); >> + e = devm_request_irq(&pdev->dev,E); >> ... >> ?-free_irq(...); >> ...+> >> } >> >> @rem depends on prb@ >> identifier platform.removefn; >> expression e; >> @@ >> removefn(...) { >> <... >> - free_irq(...); >> ...> >> } >> >> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com> >> --- >> drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c >> index 38dca69..ea4845c 100644 >> --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c >> +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c >> @@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) >> st->timer_num = ret; >> st->t = &iio_bfin_timer_code[st->timer_num]; >> >> - st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num); >> + st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d", >> + st->timer_num); >> if (!st->trig) >> return -ENOMEM; >> >> @@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) >> if (ret) >> goto out; >> >> - ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr, >> - 0, st->trig->name, st); >> + ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr, >> + 0, st->trig->name, st); >> if (ret) { >> dev_err(&pdev->dev, >> "request IRQ-%d failed", st->irq); >> @@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) >> >> ret = peripheral_request(st->t->pin, st->trig->name); >> if (ret) >> - goto out_free_irq; >> + return ret; >> >> val = (unsigned long long)get_sclk() * pdata->duty_ns; >> do_div(val, NSEC_PER_SEC); >> @@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) >> platform_set_drvdata(pdev, st); >> >> return 0; >> -out_free_irq: >> - free_irq(st->irq, st); >> + >> out1: >> iio_trigger_unregister(st->trig); >> out: >> @@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev) >> disable_gptimers(st->t->bit); >> if (st->output_enable) >> peripheral_free(st->t->pin); >> - free_irq(st->irq, st); >> iio_trigger_unregister(st->trig); >> iio_trigger_put(st->trig); >> >> -- >> 1.9.1 >> >> -- >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. >> To post to this group, send email to outreachy-kernel@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1474233410-6932-1-git-send-email-bhumirks%40gmail.com. >> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions 2016-09-19 5:53 ` Bhumika Goyal @ 2016-09-19 9:21 ` Julia Lawall [not found] ` <fc36bfeb-abae-f8ad-aca0-161cd6743137@kernel.org> 0 siblings, 1 reply; 6+ messages in thread From: Julia Lawall @ 2016-09-19 9:21 UTC (permalink / raw) To: Bhumika Goyal Cc: Alison Schofield, outreachy-kernel, jic23, knaack.h, Lars-Peter Clausen, Peter Meerwald-Stadler, gregkh@linuxfoundation.org On Mon, 19 Sep 2016, Bhumika Goyal wrote: > On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote: > > On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote: > >> Use managed resource functions devm_iio_trigger_alloc and > >> devm_request_irq instead of iio_trigger_alloc and request_irq. > >> Remove corresponding calls to free_irq in the probe and remove > >> functions. Drop the now unnecessary goto labels and replace various > >> gotos with direct returns. > >> request_irq replacement done using coccinelle. > > > > Hi Bhumika, > > Thanks for the patch. This seems like it's worthy of 2 separate > > patches. > > > > Also, I'm a bit confused with why I don't see an iio_trigger_free > > that is going away, when you switch to the devm_iio_trigger_alloc > > functions, but I realize there isn't one there to remove. hmm??? > > > > alisons > > > > Hey Alison, > Ok, I will separate this in two patches. I will do > devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as > devm_request_irq requires all previous memory allocations to be devms. > Regarding iio_trigger_free I didn't find any in either of the two > functions(probe and remove). > Thanks for the input. I think that normally people devmify everything at once. I dimly recollect that iio_trigger_unregister does the free. Check whether this is the case, and whether it is devm safe. julia > > Thanks, > Bhumika > > > > > >> > >> @platform@ > >> identifier p, probefn, removefn; > >> @@ > >> struct platform_driver p = { > >> .probe = probefn, > >> .remove = removefn, > >> }; > >> > >> @prb@ > >> identifier platform.probefn, pdev; > >> expression list E; > >> expression e; > >> > >> @@ > >> probefn(struct platform_device *pdev, ...) { > >> <+... > >> - e = request_irq(E); > >> + e = devm_request_irq(&pdev->dev,E); > >> ... > >> ?-free_irq(...); > >> ...+> > >> } > >> > >> @rem depends on prb@ > >> identifier platform.removefn; > >> expression e; > >> @@ > >> removefn(...) { > >> <... > >> - free_irq(...); > >> ...> > >> } > >> > >> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com> > >> --- > >> drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 13 ++++++------- > >> 1 file changed, 6 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c > >> index 38dca69..ea4845c 100644 > >> --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c > >> +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c > >> @@ -200,7 +200,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > >> st->timer_num = ret; > >> st->t = &iio_bfin_timer_code[st->timer_num]; > >> > >> - st->trig = iio_trigger_alloc("bfintmr%d", st->timer_num); > >> + st->trig = devm_iio_trigger_alloc(&pdev->dev, "bfintmr%d", > >> + st->timer_num); > >> if (!st->trig) > >> return -ENOMEM; > >> > >> @@ -211,8 +212,8 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > >> if (ret) > >> goto out; > >> > >> - ret = request_irq(st->irq, iio_bfin_tmr_trigger_isr, > >> - 0, st->trig->name, st); > >> + ret = devm_request_irq(&pdev->dev, st->irq, iio_bfin_tmr_trigger_isr, > >> + 0, st->trig->name, st); > >> if (ret) { > >> dev_err(&pdev->dev, > >> "request IRQ-%d failed", st->irq); > >> @@ -229,7 +230,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > >> > >> ret = peripheral_request(st->t->pin, st->trig->name); > >> if (ret) > >> - goto out_free_irq; > >> + return ret; > >> > >> val = (unsigned long long)get_sclk() * pdata->duty_ns; > >> do_div(val, NSEC_PER_SEC); > >> @@ -255,8 +256,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) > >> platform_set_drvdata(pdev, st); > >> > >> return 0; > >> -out_free_irq: > >> - free_irq(st->irq, st); > >> + > >> out1: > >> iio_trigger_unregister(st->trig); > >> out: > >> @@ -271,7 +271,6 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev) > >> disable_gptimers(st->t->bit); > >> if (st->output_enable) > >> peripheral_free(st->t->pin); > >> - free_irq(st->irq, st); > >> iio_trigger_unregister(st->trig); > >> iio_trigger_put(st->trig); > >> > >> -- > >> 1.9.1 > >> > >> -- > >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > >> To post to this group, send email to outreachy-kernel@googlegroups.com. > >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1474233410-6932-1-git-send-email-bhumirks%40gmail.com. > >> For more options, visit https://groups.google.com/d/optout. > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAOH%2B1jFsWGDVcifqDU0_9MPw%3DuEXQ%2Bu0E8dg0hkimx%2BeJbWq1Q%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <fc36bfeb-abae-f8ad-aca0-161cd6743137@kernel.org>]
[parent not found: <b02b84c9-0575-d10d-6b43-d95d167a2429@metafoo.de>]
* Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions [not found] ` <b02b84c9-0575-d10d-6b43-d95d167a2429@metafoo.de> @ 2016-10-14 19:27 ` Alison Schofield 2016-10-15 8:12 ` Bhumika Goyal 0 siblings, 1 reply; 6+ messages in thread From: Alison Schofield @ 2016-10-14 19:27 UTC (permalink / raw) To: Bhumika Goyal Cc: Jonathan Cameron, Lars-Peter Clausen, Julia Lawall, outreachy-kernel, knaack.h, Peter Meerwald-Stadler, gregkh@linuxfoundation.org, Hennerich, Michael On Mon, Sep 19, 2016 at 10:06:08PM +0200, Lars-Peter Clausen wrote: > On 09/19/2016 09:29 PM, Jonathan Cameron wrote: > > On 19/09/16 10:21, Julia Lawall wrote: > >> > >> > >> On Mon, 19 Sep 2016, Bhumika Goyal wrote: > >> > >>> On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote: > >>>> On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote: > >>>>> Use managed resource functions devm_iio_trigger_alloc and > >>>>> devm_request_irq instead of iio_trigger_alloc and request_irq. > >>>>> Remove corresponding calls to free_irq in the probe and remove > >>>>> functions. Drop the now unnecessary goto labels and replace various > >>>>> gotos with direct returns. > >>>>> request_irq replacement done using coccinelle. > >>>> > >>>> Hi Bhumika, > >>>> Thanks for the patch. This seems like it's worthy of 2 separate > >>>> patches. > >>>> > >>>> Also, I'm a bit confused with why I don't see an iio_trigger_free > >>>> that is going away, when you switch to the devm_iio_trigger_alloc > >>>> functions, but I realize there isn't one there to remove. hmm??? > >>>> > >>>> alisons > >>>> > >>> > >>> Hey Alison, > >>> Ok, I will separate this in two patches. I will do > >>> devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as > >>> devm_request_irq requires all previous memory allocations to be devms. > >>> Regarding iio_trigger_free I didn't find any in either of the two > >>> functions(probe and remove). > >>> Thanks for the input. > >> > >> I think that normally people devmify everything at once. > > I would as well. It is 'kind of' one change... > >> > >> I dimly recollect that iio_trigger_unregister does the free. Check > >> whether this is the case, and whether it is devm safe. > > Given it's referenced on the next line I certainly hope it doesn't ;) > > There are quite a few reasons this one is in staging! > > For some reason this driver uses iio_trigger_put() instead of > iio_trigger_free(). Which is wrong, since iio_trigger_put() does an > additional module_put() which would release a reference we've never gained. > So the first fix should be to replace iio_trigger_put() with > iio_trigger_free() (which is a patch for stable) and then switch to the devm > version and remove the iio_trigger_free() (which is a patch for next). > > This does not seem to be the only driver that gets this wrong, > iio-trig-sysfs and iio-trig-interrupt are also affected. These would make a > good target for further patches. > > Bonus points if you can present a proof-of-concept that exploits this bug to > run arbitrary code with kernel privileges. > > There seems to be another bug regarding trigger reference counting in > iio_trigger_write_current(). The reference to the trigger should be acquired > in iio_trigger_find_by_name() while holding mutex. Otherwise we risk that > the trigger is already freed before we grab the reference later in > iio_trigger_write_current(). That's another opportunity for sending a good > patch and also some opportunity for bonus points. > Hi Bhumika, I have interest in working this issue. Let me know if it's ok for me to run with. Thanks! alisons ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions 2016-10-14 19:27 ` Alison Schofield @ 2016-10-15 8:12 ` Bhumika Goyal 0 siblings, 0 replies; 6+ messages in thread From: Bhumika Goyal @ 2016-10-15 8:12 UTC (permalink / raw) To: Alison Schofield Cc: Jonathan Cameron, Lars-Peter Clausen, Julia Lawall, outreachy-kernel, knaack.h, Peter Meerwald-Stadler, gregkh@linuxfoundation.org, Hennerich, Michael On Sat, Oct 15, 2016 at 12:57 AM, Alison Schofield <amsfield22@gmail.com> wrote: > On Mon, Sep 19, 2016 at 10:06:08PM +0200, Lars-Peter Clausen wrote: >> On 09/19/2016 09:29 PM, Jonathan Cameron wrote: >> > On 19/09/16 10:21, Julia Lawall wrote: >> >> >> >> >> >> On Mon, 19 Sep 2016, Bhumika Goyal wrote: >> >> >> >>> On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote: >> >>>> On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote: >> >>>>> Use managed resource functions devm_iio_trigger_alloc and >> >>>>> devm_request_irq instead of iio_trigger_alloc and request_irq. >> >>>>> Remove corresponding calls to free_irq in the probe and remove >> >>>>> functions. Drop the now unnecessary goto labels and replace various >> >>>>> gotos with direct returns. >> >>>>> request_irq replacement done using coccinelle. >> >>>> >> >>>> Hi Bhumika, >> >>>> Thanks for the patch. This seems like it's worthy of 2 separate >> >>>> patches. >> >>>> >> >>>> Also, I'm a bit confused with why I don't see an iio_trigger_free >> >>>> that is going away, when you switch to the devm_iio_trigger_alloc >> >>>> functions, but I realize there isn't one there to remove. hmm??? >> >>>> >> >>>> alisons >> >>>> >> >>> >> >>> Hey Alison, >> >>> Ok, I will separate this in two patches. I will do >> >>> devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as >> >>> devm_request_irq requires all previous memory allocations to be devms. >> >>> Regarding iio_trigger_free I didn't find any in either of the two >> >>> functions(probe and remove). >> >>> Thanks for the input. >> >> >> >> I think that normally people devmify everything at once. >> > I would as well. It is 'kind of' one change... >> >> >> >> I dimly recollect that iio_trigger_unregister does the free. Check >> >> whether this is the case, and whether it is devm safe. >> > Given it's referenced on the next line I certainly hope it doesn't ;) >> > There are quite a few reasons this one is in staging! >> >> For some reason this driver uses iio_trigger_put() instead of >> iio_trigger_free(). Which is wrong, since iio_trigger_put() does an >> additional module_put() which would release a reference we've never gained. >> So the first fix should be to replace iio_trigger_put() with >> iio_trigger_free() (which is a patch for stable) and then switch to the devm >> version and remove the iio_trigger_free() (which is a patch for next). >> >> This does not seem to be the only driver that gets this wrong, >> iio-trig-sysfs and iio-trig-interrupt are also affected. These would make a >> good target for further patches. >> >> Bonus points if you can present a proof-of-concept that exploits this bug to >> run arbitrary code with kernel privileges. >> >> There seems to be another bug regarding trigger reference counting in >> iio_trigger_write_current(). The reference to the trigger should be acquired >> in iio_trigger_find_by_name() while holding mutex. Otherwise we risk that >> the trigger is already freed before we grab the reference later in >> iio_trigger_write_current(). That's another opportunity for sending a good >> patch and also some opportunity for bonus points. >> > > Hi Bhumika, > I have interest in working this issue. Let me know if it's ok for > me to run with. > Thanks! Hey Alison, You can surely go ahead with this issue :) Thanks, Bhumika > alisons ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-15 8:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-18 21:16 [PATCH] Staging: iio: trigger: Use devm functions Bhumika Goyal
2016-09-19 5:44 ` [Outreachy kernel] " Alison Schofield
2016-09-19 5:53 ` Bhumika Goyal
2016-09-19 9:21 ` Julia Lawall
[not found] ` <fc36bfeb-abae-f8ad-aca0-161cd6743137@kernel.org>
[not found] ` <b02b84c9-0575-d10d-6b43-d95d167a2429@metafoo.de>
2016-10-14 19:27 ` Alison Schofield
2016-10-15 8:12 ` Bhumika Goyal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.