Using the `devm_` variant for requesting IRQ _before_ the `devm_`
variant for allocating/registering the `power_supply` handle, means that
the `power_supply` handle will be deallocated/unregistered _before_ the
interrupt handler (since `devm_` naturally deallocates in reverse
allocation order). This means that during removal, there is a race
condition where an interrupt can fire just _after_ the `power_supply`
handle has been freed, *but* just _before_ the corresponding
unregistration of the IRQ handler has run.

This will lead to the IRQ handler calling `power_supply_changed()` with
a freed `power_supply` handle. Which usually crashes the system or
otherwise silently corrupts the memory...

Note that there is a similar situation which can also happen during
`probe()`; the possibility of an interrupt firing _before_ registering
the `power_supply` handle. This would then lead to the nasty situation
of using the `power_supply` handle *uninitialized* in
`power_supply_changed()`.

Fix this racy use-after-free by making sure the IRQ is requested _after_
the registration of the `power_supply` handle.

Fixes: f8d7a3d21160 ("power: supply: Add driver for pm8916 lbc")
Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 drivers/power/supply/pm8916_lbc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/pm8916_lbc.c b/drivers/power/supply/pm8916_lbc.c
index c74b75b1b2676..3ca717d84aade 100644
--- a/drivers/power/supply/pm8916_lbc.c
+++ b/drivers/power/supply/pm8916_lbc.c
@@ -274,15 +274,6 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, -EINVAL,
 				     "Wrong amount of reg values: %d (4 expected)\n", len);
 
-	irq = platform_get_irq_byname(pdev, "usb_vbus");
-	if (irq < 0)
-		return irq;
-
-	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
-					IRQF_ONESHOT, "pm8916_lbc", chg);
-	if (ret)
-		return ret;
-
 	ret = device_property_read_u32_array(dev, "reg", chg->reg, len);
 	if (ret)
 		return ret;
@@ -332,6 +323,15 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Unable to get battery info\n");
 
+	irq = platform_get_irq_byname(pdev, "usb_vbus");
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
+					IRQF_ONESHOT, "pm8916_lbc", chg);
+	if (ret)
+		return ret;
+
 	chg->edev = devm_extcon_dev_allocate(dev, pm8916_lbc_charger_cable);
 	if (IS_ERR(chg->edev))
 		return PTR_ERR(chg->edev);
-- 
2.39.5

Waqar Hameed писал(а) 21.12.2025 03:36:
> Using the `devm_` variant for requesting IRQ _before_ the `devm_`
> variant for allocating/registering the `power_supply` handle, means that
> the `power_supply` handle will be deallocated/unregistered _before_ the
> interrupt handler (since `devm_` naturally deallocates in reverse
> allocation order). This means that during removal, there is a race
> condition where an interrupt can fire just _after_ the `power_supply`
> handle has been freed, *but* just _before_ the corresponding
> unregistration of the IRQ handler has run.
> 
> This will lead to the IRQ handler calling `power_supply_changed()` with
> a freed `power_supply` handle. Which usually crashes the system or
> otherwise silently corrupts the memory...
> 
> Note that there is a similar situation which can also happen during
> `probe()`; the possibility of an interrupt firing _before_ registering
> the `power_supply` handle. This would then lead to the nasty situation
> of using the `power_supply` handle *uninitialized* in
> `power_supply_changed()`.
> 
> Fix this racy use-after-free by making sure the IRQ is requested _after_
> the registration of the `power_supply` handle.
> 
> Fixes: f8d7a3d21160 ("power: supply: Add driver for pm8916 lbc")
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
> ---
>  drivers/power/supply/pm8916_lbc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/supply/pm8916_lbc.c b/drivers/power/supply/pm8916_lbc.c
> index c74b75b1b2676..3ca717d84aade 100644
> --- a/drivers/power/supply/pm8916_lbc.c
> +++ b/drivers/power/supply/pm8916_lbc.c
> @@ -274,15 +274,6 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "Wrong amount of reg values: %d (4 expected)\n", len);
>  
> -	irq = platform_get_irq_byname(pdev, "usb_vbus");
> -	if (irq < 0)
> -		return irq;
> -
> -	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
> -					IRQF_ONESHOT, "pm8916_lbc", chg);
> -	if (ret)
> -		return ret;
> -
>  	ret = device_property_read_u32_array(dev, "reg", chg->reg, len);
>  	if (ret)
>  		return ret;
> @@ -332,6 +323,15 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Unable to get battery info\n");
>  
> +	irq = platform_get_irq_byname(pdev, "usb_vbus");
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
> +					IRQF_ONESHOT, "pm8916_lbc", chg);
> +	if (ret)
> +		return ret;
> +

Thank you for looking at those drivers and fixing this!

As a small note, the interrupt handler also has a call to
extcon_set_state_sync(chg->edev,...) which is allocated right below.
I don't think this is actually a problem since it has a null check for
edev (unlike psy core) so I think this patch is fine as-is. However if
for some reason you'd have to respin this series, perhaps it would be
nice to move irq registration slightly lower, after extcon registration.

In any case,

Reviewed-by: Nikita Travkin <nikita@trvn.ru>

Nikita

>  	chg->edev = devm_extcon_dev_allocate(dev, pm8916_lbc_charger_cable);
>  	if (IS_ERR(chg->edev))
>  		return PTR_ERR(chg->edev);

On Sun, Dec 21, 2025 at 10:45 +0500 Nikita Travkin <nikita@trvn.ru> wrote:

> Waqar Hameed писал(а) 21.12.2025 03:36:
>> Using the `devm_` variant for requesting IRQ _before_ the `devm_`
>> variant for allocating/registering the `power_supply` handle, means that
>> the `power_supply` handle will be deallocated/unregistered _before_ the
>> interrupt handler (since `devm_` naturally deallocates in reverse
>> allocation order). This means that during removal, there is a race
>> condition where an interrupt can fire just _after_ the `power_supply`
>> handle has been freed, *but* just _before_ the corresponding
>> unregistration of the IRQ handler has run.
>> 
>> This will lead to the IRQ handler calling `power_supply_changed()` with
>> a freed `power_supply` handle. Which usually crashes the system or
>> otherwise silently corrupts the memory...
>> 
>> Note that there is a similar situation which can also happen during
>> `probe()`; the possibility of an interrupt firing _before_ registering
>> the `power_supply` handle. This would then lead to the nasty situation
>> of using the `power_supply` handle *uninitialized* in
>> `power_supply_changed()`.
>> 
>> Fix this racy use-after-free by making sure the IRQ is requested _after_
>> the registration of the `power_supply` handle.
>> 
>> Fixes: f8d7a3d21160 ("power: supply: Add driver for pm8916 lbc")
>> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
>> ---
>>  drivers/power/supply/pm8916_lbc.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/power/supply/pm8916_lbc.c b/drivers/power/supply/pm8916_lbc.c
>> index c74b75b1b2676..3ca717d84aade 100644
>> --- a/drivers/power/supply/pm8916_lbc.c
>> +++ b/drivers/power/supply/pm8916_lbc.c
>> @@ -274,15 +274,6 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
>>  		return dev_err_probe(dev, -EINVAL,
>>  				     "Wrong amount of reg values: %d (4 expected)\n", len);
>>  
>> -	irq = platform_get_irq_byname(pdev, "usb_vbus");
>> -	if (irq < 0)
>> -		return irq;
>> -
>> -	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
>> -					IRQF_ONESHOT, "pm8916_lbc", chg);
>> -	if (ret)
>> -		return ret;
>> -
>>  	ret = device_property_read_u32_array(dev, "reg", chg->reg, len);
>>  	if (ret)
>>  		return ret;
>> @@ -332,6 +323,15 @@ static int pm8916_lbc_charger_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return dev_err_probe(dev, ret, "Unable to get battery info\n");
>>  
>> +	irq = platform_get_irq_byname(pdev, "usb_vbus");
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
>> +					IRQF_ONESHOT, "pm8916_lbc", chg);
>> +	if (ret)
>> +		return ret;
>> +
>
> Thank you for looking at those drivers and fixing this!

Thank _you_ for reviewing!

>
> As a small note, the interrupt handler also has a call to
> extcon_set_state_sync(chg->edev,...) which is allocated right below.
> I don't think this is actually a problem since it has a null check for
> edev (unlike psy core) so I think this patch is fine as-is. However if
> for some reason you'd have to respin this series, perhaps it would be
> nice to move irq registration slightly lower, after extcon registration.

Hm, it _is_ actually a problem. During `probe()`, it's fine, due to the
NULL check in `extcon_set_state()` (and the interrupt handler doesn't
check the return value anyway), as you mention. However, during removal,
we have the exact same situation as for `power_supply_changed()` as
explained in the commit message; `devm_extcon_dev_release()` runs and
frees `struct extcon_dev *edev`, the interrupt handler would now call

  `extcon_set_state_sync(chg->edev, ...)` ->
  `extcon_set_state(edev, ...)` ->
  `find_cable_index_by_id(edev, ...)`
  
with an invalid `edev` triggering a crash/corruption in
`find_cable_index_by_id()` (before we get the chance to release the IRQ
handler)!

Good catch! Let's move the registration a further bit down to fix this.
I will send v2 as soon as the other patches in the series also get
feedback.

>
> In any case,
>
> Reviewed-by: Nikita Travkin <nikita@trvn.ru>
>
> Nikita
>
>>  	chg->edev = devm_extcon_dev_allocate(dev, pm8916_lbc_charger_cable);
>>  	if (IS_ERR(chg->edev))
>>  		return PTR_ERR(chg->edev);

On Wed, Jan 07, 2026 at 15:32 +0100 Waqar Hameed <waqar.hameed@axis.com> wrote:

> On Sun, Dec 21, 2025 at 10:45 +0500 Nikita Travkin <nikita@trvn.ru> wrote:

[...]

>> As a small note, the interrupt handler also has a call to
>> extcon_set_state_sync(chg->edev,...) which is allocated right below.
>> I don't think this is actually a problem since it has a null check for
>> edev (unlike psy core) so I think this patch is fine as-is. However if
>> for some reason you'd have to respin this series, perhaps it would be
>> nice to move irq registration slightly lower, after extcon registration.
>
> Hm, it _is_ actually a problem. During `probe()`, it's fine, due to the
> NULL check in `extcon_set_state()` (and the interrupt handler doesn't
> check the return value anyway), as you mention. However, during removal,
> we have the exact same situation as for `power_supply_changed()` as
> explained in the commit message; `devm_extcon_dev_release()` runs and
> frees `struct extcon_dev *edev`, the interrupt handler would now call
>
>   `extcon_set_state_sync(chg->edev, ...)` ->
>   `extcon_set_state(edev, ...)` ->
>   `find_cable_index_by_id(edev, ...)`
>   
> with an invalid `edev` triggering a crash/corruption in
> `find_cable_index_by_id()` (before we get the chance to release the IRQ
> handler)!
>
> Good catch! Let's move the registration a further bit down to fix this.
> I will send v2 as soon as the other patches in the series also get
> feedback.

Since Sebastian says that he applied the whole series as is, I'll just
send a new separate patch for that now instead. I couldn't find anything
in his tree [1] yet so let's chill a bit until things get pushed.

[1] git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git

Hi,

On Wed, Jan 14, 2026 at 11:48:06AM +0100, Waqar Hameed wrote:
> On Wed, Jan 07, 2026 at 15:32 +0100 Waqar Hameed <waqar.hameed@axis.com> wrote:
> 
> > On Sun, Dec 21, 2025 at 10:45 +0500 Nikita Travkin <nikita@trvn.ru> wrote:
> 
> [...]
> 
> >> As a small note, the interrupt handler also has a call to
> >> extcon_set_state_sync(chg->edev,...) which is allocated right below.
> >> I don't think this is actually a problem since it has a null check for
> >> edev (unlike psy core) so I think this patch is fine as-is. However if
> >> for some reason you'd have to respin this series, perhaps it would be
> >> nice to move irq registration slightly lower, after extcon registration.
> >
> > Hm, it _is_ actually a problem. During `probe()`, it's fine, due to the
> > NULL check in `extcon_set_state()` (and the interrupt handler doesn't
> > check the return value anyway), as you mention. However, during removal,
> > we have the exact same situation as for `power_supply_changed()` as
> > explained in the commit message; `devm_extcon_dev_release()` runs and
> > frees `struct extcon_dev *edev`, the interrupt handler would now call
> >
> >   `extcon_set_state_sync(chg->edev, ...)` ->
> >   `extcon_set_state(edev, ...)` ->
> >   `find_cable_index_by_id(edev, ...)`
> >   
> > with an invalid `edev` triggering a crash/corruption in
> > `find_cable_index_by_id()` (before we get the chance to release the IRQ
> > handler)!
> >
> > Good catch! Let's move the registration a further bit down to fix this.
> > I will send v2 as soon as the other patches in the series also get
> > feedback.
> 
> Since Sebastian says that he applied the whole series as is, I'll just
> send a new separate patch for that now instead. I couldn't find anything
> in his tree [1] yet so let's chill a bit until things get pushed.

Ah yes, I planed to reply to this and ask for doing exactly that and
then forgot about it :)

FWIW you can find this patch here:

https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next&id=b7508129978ae1e2ed9b0410396abc05def9c4eb

Greetings,

-- Sebastian