* [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error @ 2021-10-31 15:25 Hans de Goede 2021-10-31 19:34 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2021-10-31 15:25 UTC (permalink / raw) To: Sebastian Reichel Cc: Hans de Goede, linux-acpi, platform-driver-x86, linux-pm, Andrew F . Davis When registering the IRQ handler fails, do not just return the error code, this will free the devm_kzalloc()-ed data struct while leaving the queued work queued and the registered power_supply registered with both of them now pointing to free-ed memory, resulting in various kernel crashes soon afterwards. Instead properly tear-down things on IRQ handler register errors. Fixes: 703df6c09795 ("power: bq27xxx_battery: Reorganize I2C into a module") Cc: Andrew F. Davis <afd@ti.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Fix devm_kzalloc()-ed type in the commit message --- drivers/power/supply/bq27xxx_battery_i2c.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c index 46f078350fd3..cf38cbfe13e9 100644 --- a/drivers/power/supply/bq27xxx_battery_i2c.c +++ b/drivers/power/supply/bq27xxx_battery_i2c.c @@ -187,7 +187,8 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client, dev_err(&client->dev, "Unable to register IRQ %d error %d\n", client->irq, ret); - return ret; + bq27xxx_battery_teardown(di); + goto err_failed; } } -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error 2021-10-31 15:25 [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error Hans de Goede @ 2021-10-31 19:34 ` Andy Shevchenko 2021-11-02 13:23 ` Sebastian Reichel 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2021-10-31 19:34 UTC (permalink / raw) To: Hans de Goede Cc: Sebastian Reichel, ACPI Devel Maling List, Platform Driver, Linux PM, Andrew F . Davis On Sun, Oct 31, 2021 at 5:25 PM Hans de Goede <hdegoede@redhat.com> wrote: > > When registering the IRQ handler fails, do not just return the error code, > this will free the devm_kzalloc()-ed data struct while leaving the queued > work queued and the registered power_supply registered with both of them > now pointing to free-ed memory, resulting in various kernel crashes > soon afterwards. > > Instead properly tear-down things on IRQ handler register errors. FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Fixes: 703df6c09795 ("power: bq27xxx_battery: Reorganize I2C into a module") > Cc: Andrew F. Davis <afd@ti.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > - Fix devm_kzalloc()-ed type in the commit message > --- > drivers/power/supply/bq27xxx_battery_i2c.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c > index 46f078350fd3..cf38cbfe13e9 100644 > --- a/drivers/power/supply/bq27xxx_battery_i2c.c > +++ b/drivers/power/supply/bq27xxx_battery_i2c.c > @@ -187,7 +187,8 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client, > dev_err(&client->dev, > "Unable to register IRQ %d error %d\n", > client->irq, ret); > - return ret; > + bq27xxx_battery_teardown(di); > + goto err_failed; > } > } > > -- > 2.31.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error 2021-10-31 19:34 ` Andy Shevchenko @ 2021-11-02 13:23 ` Sebastian Reichel 2021-11-02 14:12 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Sebastian Reichel @ 2021-11-02 13:23 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, ACPI Devel Maling List, Platform Driver, Linux PM, Andrew F . Davis [-- Attachment #1: Type: text/plain, Size: 1861 bytes --] Hi, On Sun, Oct 31, 2021 at 09:34:46PM +0200, Andy Shevchenko wrote: > On Sun, Oct 31, 2021 at 5:25 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > When registering the IRQ handler fails, do not just return the error code, > > this will free the devm_kzalloc()-ed data struct while leaving the queued > > work queued and the registered power_supply registered with both of them > > now pointing to free-ed memory, resulting in various kernel crashes > > soon afterwards. > > > > Instead properly tear-down things on IRQ handler register errors. > > FWIW, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Thanks, queued. -- Sebastian > > Fixes: 703df6c09795 ("power: bq27xxx_battery: Reorganize I2C into a module") > > Cc: Andrew F. Davis <afd@ti.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > Changes in v2: > > - Fix devm_kzalloc()-ed type in the commit message > > --- > > drivers/power/supply/bq27xxx_battery_i2c.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c > > index 46f078350fd3..cf38cbfe13e9 100644 > > --- a/drivers/power/supply/bq27xxx_battery_i2c.c > > +++ b/drivers/power/supply/bq27xxx_battery_i2c.c > > @@ -187,7 +187,8 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client, > > dev_err(&client->dev, > > "Unable to register IRQ %d error %d\n", > > client->irq, ret); > > - return ret; > > + bq27xxx_battery_teardown(di); > > + goto err_failed; > > } > > } > > > > -- > > 2.31.1 > > > > > -- > With Best Regards, > Andy Shevchenko [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error 2021-11-02 13:23 ` Sebastian Reichel @ 2021-11-02 14:12 ` Hans de Goede 2021-11-02 16:41 ` Sebastian Reichel 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2021-11-02 14:12 UTC (permalink / raw) To: Sebastian Reichel, Andy Shevchenko Cc: ACPI Devel Maling List, Platform Driver, Linux PM, Andrew F . Davis Hi Sebastian, On 11/2/21 14:23, Sebastian Reichel wrote: > Hi, > > On Sun, Oct 31, 2021 at 09:34:46PM +0200, Andy Shevchenko wrote: >> On Sun, Oct 31, 2021 at 5:25 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> When registering the IRQ handler fails, do not just return the error code, >>> this will free the devm_kzalloc()-ed data struct while leaving the queued >>> work queued and the registered power_supply registered with both of them >>> now pointing to free-ed memory, resulting in various kernel crashes >>> soon afterwards. >>> >>> Instead properly tear-down things on IRQ handler register errors. >> >> FWIW, >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Thanks, queued. Thank you, note these 2 patches (for the bq25980 driver) are also pure bug-fixes, I posted them as part of a larger series, but in hindsight I should have probably posted them separately: https://lore.kernel.org/platform-driver-x86/20211030182813.116672-4-hdegoede@redhat.com/ https://lore.kernel.org/platform-driver-x86/20211030182813.116672-5-hdegoede@redhat.com/ It would be good if you can pick these 2 up too (I'll respin the rest of the series to address various review comments without them then). Let me know if you want me to resend these 2 as a stand alone series. Regards, Hans >>> Fixes: 703df6c09795 ("power: bq27xxx_battery: Reorganize I2C into a module") >>> Cc: Andrew F. Davis <afd@ti.com> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> Changes in v2: >>> - Fix devm_kzalloc()-ed type in the commit message >>> --- >>> drivers/power/supply/bq27xxx_battery_i2c.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c >>> index 46f078350fd3..cf38cbfe13e9 100644 >>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c >>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c >>> @@ -187,7 +187,8 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client, >>> dev_err(&client->dev, >>> "Unable to register IRQ %d error %d\n", >>> client->irq, ret); >>> - return ret; >>> + bq27xxx_battery_teardown(di); >>> + goto err_failed; >>> } >>> } >>> >>> -- >>> 2.31.1 >>> >> >> >> -- >> With Best Regards, >> Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error 2021-11-02 14:12 ` Hans de Goede @ 2021-11-02 16:41 ` Sebastian Reichel 2021-11-02 17:58 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Sebastian Reichel @ 2021-11-02 16:41 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, ACPI Devel Maling List, Platform Driver, Linux PM, Andrew F . Davis [-- Attachment #1: Type: text/plain, Size: 1740 bytes --] Hi Hans, On Tue, Nov 02, 2021 at 03:12:51PM +0100, Hans de Goede wrote: > Hi Sebastian, > > On 11/2/21 14:23, Sebastian Reichel wrote: > > Hi, > > > > On Sun, Oct 31, 2021 at 09:34:46PM +0200, Andy Shevchenko wrote: > >> On Sun, Oct 31, 2021 at 5:25 PM Hans de Goede <hdegoede@redhat.com> wrote: > >>> > >>> When registering the IRQ handler fails, do not just return the error code, > >>> this will free the devm_kzalloc()-ed data struct while leaving the queued > >>> work queued and the registered power_supply registered with both of them > >>> now pointing to free-ed memory, resulting in various kernel crashes > >>> soon afterwards. > >>> > >>> Instead properly tear-down things on IRQ handler register errors. > >> > >> FWIW, > >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Thanks, queued. > > Thank you, note these 2 patches (for the bq25980 driver) are also > pure bug-fixes, I posted them as part of a larger series, but > in hindsight I should have probably posted them separately: > > https://lore.kernel.org/platform-driver-x86/20211030182813.116672-4-hdegoede@redhat.com/ > https://lore.kernel.org/platform-driver-x86/20211030182813.116672-5-hdegoede@redhat.com/ > > It would be good if you can pick these 2 up too > (I'll respin the rest of the series to address various > review comments without them then). > Let me know if you want me to resend these 2 as a stand alone > series. Thanks for the pointer, I queued both of them. I had to slightly rebase, since your base did not include 172d0ccea55c. I have not yet reviewed the remaining patchset, but considering there are more patches to bq25890 a rebase would be ncie. Greetings, -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error 2021-11-02 16:41 ` Sebastian Reichel @ 2021-11-02 17:58 ` Hans de Goede 0 siblings, 0 replies; 6+ messages in thread From: Hans de Goede @ 2021-11-02 17:58 UTC (permalink / raw) To: Sebastian Reichel Cc: Andy Shevchenko, ACPI Devel Maling List, Platform Driver, Linux PM, Andrew F . Davis Hi, On 11/2/21 17:41, Sebastian Reichel wrote: > Hi Hans, > > On Tue, Nov 02, 2021 at 03:12:51PM +0100, Hans de Goede wrote: >> Hi Sebastian, >> >> On 11/2/21 14:23, Sebastian Reichel wrote: >>> Hi, >>> >>> On Sun, Oct 31, 2021 at 09:34:46PM +0200, Andy Shevchenko wrote: >>>> On Sun, Oct 31, 2021 at 5:25 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>>> >>>>> When registering the IRQ handler fails, do not just return the error code, >>>>> this will free the devm_kzalloc()-ed data struct while leaving the queued >>>>> work queued and the registered power_supply registered with both of them >>>>> now pointing to free-ed memory, resulting in various kernel crashes >>>>> soon afterwards. >>>>> >>>>> Instead properly tear-down things on IRQ handler register errors. >>>> >>>> FWIW, >>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>> >>> Thanks, queued. >> >> Thank you, note these 2 patches (for the bq25980 driver) are also >> pure bug-fixes, I posted them as part of a larger series, but >> in hindsight I should have probably posted them separately: >> >> https://lore.kernel.org/platform-driver-x86/20211030182813.116672-4-hdegoede@redhat.com/ >> https://lore.kernel.org/platform-driver-x86/20211030182813.116672-5-hdegoede@redhat.com/ >> >> It would be good if you can pick these 2 up too >> (I'll respin the rest of the series to address various >> review comments without them then). >> Let me know if you want me to resend these 2 as a stand alone >> series. > > Thanks for the pointer, I queued both of them. I had to slightly > rebase, since your base did not include 172d0ccea55c. I have not > yet reviewed the remaining patchset, but considering there are more > patches to bq25890 a rebase would be ncie. Ack, I'll rebase for the next version of that series. Regards, Hans ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-02 17:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-31 15:25 [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error Hans de Goede 2021-10-31 19:34 ` Andy Shevchenko 2021-11-02 13:23 ` Sebastian Reichel 2021-11-02 14:12 ` Hans de Goede 2021-11-02 16:41 ` Sebastian Reichel 2021-11-02 17:58 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).