| Message ID | 20211031152522.3911-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error | expand |
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 >
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
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
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
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
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; } }
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(-)