[PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error

[PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error

[Hans de Goede]

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

Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error

[Andy Shevchenko]

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

Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error

[Sebastian Reichel]

[-- 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 --]

Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error

[Hans de Goede]

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

Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error

[Sebastian Reichel]

[-- 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 --]

Re: [PATCH v2] power: supply: bq27xxx: Fix kernel crash on IRQ handler register error

[Hans de Goede]

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