[PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Qiu Wenbo]

From: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>

regulator_unregister() already frees the associated GPIO device. On
ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
random failures when other drivers (typically Intel THC) attempt to
allocate interrupts. The root cause is that the reference count of the
pinctrl_intel_platform module unexpectedly drops to zero when this
driver defers its probe.

This behavior can also be reproduced by unloading the module directly.

Fix the issue by removing the redundant release of the GPIO device
during regulator unregistration.

Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")

Signed-off-by: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 5 +----
 include/linux/platform_data/x86/int3472.h              | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 476ec24d37020..9e052b164a1ab 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -245,15 +245,12 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	if (IS_ERR(regulator->rdev))
 		return PTR_ERR(regulator->rdev);
 
-	int3472->regulators[int3472->n_regulator_gpios].ena_gpio = gpio;
 	int3472->n_regulator_gpios++;
 	return 0;
 }
 
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472)
 {
-	for (int i = 0; i < int3472->n_regulator_gpios; i++) {
+	for (int i = 0; i < int3472->n_regulator_gpios; i++)
 		regulator_unregister(int3472->regulators[i].rdev);
-		gpiod_put(int3472->regulators[i].ena_gpio);
-	}
 }
diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
index 1571e9157fa50..b1b837583d544 100644
--- a/include/linux/platform_data/x86/int3472.h
+++ b/include/linux/platform_data/x86/int3472.h
@@ -100,7 +100,6 @@ struct int3472_gpio_regulator {
 	struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
 	char supply_name_upper[GPIO_SUPPLY_NAME_LENGTH];
 	char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
-	struct gpio_desc *ena_gpio;
 	struct regulator_dev *rdev;
 	struct regulator_desc rdesc;
 };
-- 
2.51.1

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Andy Shevchenko]

On Fri, Oct 24, 2025 at 01:05:37PM +0800, Qiu Wenbo wrote:
> 
> regulator_unregister() already frees the associated GPIO device. On
> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
> random failures when other drivers (typically Intel THC) attempt to
> allocate interrupts. The root cause is that the reference count of the
> pinctrl_intel_platform module unexpectedly drops to zero when this
> driver defers its probe.
> 
> This behavior can also be reproduced by unloading the module directly.
> 
> Fix the issue by removing the redundant release of the GPIO device
> during regulator unregistration.
> 
> Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")

> 

Should not be a blank line here.

> Signed-off-by: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
> ---

The change sounds reasonable to me. FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Sakari Ailus]

Hi Qiu,

Thanks for the patch.

On Fri, Oct 24, 2025 at 01:05:37PM +0800, Qiu Wenbo wrote:
> From: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
> 
> regulator_unregister() already frees the associated GPIO device. On
> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
> random failures when other drivers (typically Intel THC) attempt to
> allocate interrupts. The root cause is that the reference count of the
> pinctrl_intel_platform module unexpectedly drops to zero when this
> driver defers its probe.
> 
> This behavior can also be reproduced by unloading the module directly.
> 
> Fix the issue by removing the redundant release of the GPIO device
> during regulator unregistration.
> 
> Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")

Shouldn't this be also cc'd to stable@

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Regards,

Sakari Ailus

[PATCH v2] platform/x86: int3472: Fix double free of GPIO device during unregister

[Qiu Wenbo]

From: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>

regulator_unregister() already frees the associated GPIO device. On
ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
random failures when other drivers (typically Intel THC) attempt to
allocate interrupts. The root cause is that the reference count of the
pinctrl_intel_platform module unexpectedly drops to zero when this
driver defers its probe.

This behavior can also be reproduced by unloading the module directly.

Fix the issue by removing the redundant release of the GPIO device
during regulator unregistration.

Cc: stable@vger.kernel.org
Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")
Signed-off-by: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Changes since V1:
 - Add Reviewed-by: from Andy Shevchenko and Sakari Ailus
 - Add Cc to stable@
 - Remove the blank line after Fixes:

 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 5 +----
 include/linux/platform_data/x86/int3472.h              | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 476ec24d37020..9e052b164a1ab 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -245,15 +245,12 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	if (IS_ERR(regulator->rdev))
 		return PTR_ERR(regulator->rdev);
 
-	int3472->regulators[int3472->n_regulator_gpios].ena_gpio = gpio;
 	int3472->n_regulator_gpios++;
 	return 0;
 }
 
 void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472)
 {
-	for (int i = 0; i < int3472->n_regulator_gpios; i++) {
+	for (int i = 0; i < int3472->n_regulator_gpios; i++)
 		regulator_unregister(int3472->regulators[i].rdev);
-		gpiod_put(int3472->regulators[i].ena_gpio);
-	}
 }
diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
index 1571e9157fa50..b1b837583d544 100644
--- a/include/linux/platform_data/x86/int3472.h
+++ b/include/linux/platform_data/x86/int3472.h
@@ -100,7 +100,6 @@ struct int3472_gpio_regulator {
 	struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
 	char supply_name_upper[GPIO_SUPPLY_NAME_LENGTH];
 	char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
-	struct gpio_desc *ena_gpio;
 	struct regulator_dev *rdev;
 	struct regulator_desc rdesc;
 };
-- 
2.51.2

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Dan Scally]

Hi Qiu

On 24/10/2025 06:05, Qiu Wenbo wrote:
> From: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
> 
> regulator_unregister() already frees the associated GPIO device. On
> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
> random failures when other drivers (typically Intel THC) attempt to
> allocate interrupts. The root cause is that the reference count of the
> pinctrl_intel_platform module unexpectedly drops to zero when this
> driver defers its probe.
> 
> This behavior can also be reproduced by unloading the module directly.
> 
> Fix the issue by removing the redundant release of the GPIO device
> during regulator unregistration.
> 
> Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")
> 
> Signed-off-by: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
> ---

The change looks good to me, so:

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

However the Fixes tag I wonder about; devm_gpiod_get() will also result in a call to gpiod_put() 
when the module is unloaded; doesn't that mean that the same issue  will occur before that commit?

Thanks
Dan



>   drivers/platform/x86/intel/int3472/clk_and_regulator.c | 5 +----
>   include/linux/platform_data/x86/int3472.h              | 1 -
>   2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> index 476ec24d37020..9e052b164a1ab 100644
> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> @@ -245,15 +245,12 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>   	if (IS_ERR(regulator->rdev))
>   		return PTR_ERR(regulator->rdev);
>   
> -	int3472->regulators[int3472->n_regulator_gpios].ena_gpio = gpio;
>   	int3472->n_regulator_gpios++;
>   	return 0;
>   }
>   
>   void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472)
>   {
> -	for (int i = 0; i < int3472->n_regulator_gpios; i++) {
> +	for (int i = 0; i < int3472->n_regulator_gpios; i++)
>   		regulator_unregister(int3472->regulators[i].rdev);
> -		gpiod_put(int3472->regulators[i].ena_gpio);
> -	}
>   }
> diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
> index 1571e9157fa50..b1b837583d544 100644
> --- a/include/linux/platform_data/x86/int3472.h
> +++ b/include/linux/platform_data/x86/int3472.h
> @@ -100,7 +100,6 @@ struct int3472_gpio_regulator {
>   	struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
>   	char supply_name_upper[GPIO_SUPPLY_NAME_LENGTH];
>   	char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
> -	struct gpio_desc *ena_gpio;
>   	struct regulator_dev *rdev;
>   	struct regulator_desc rdesc;
>   };

Re: [PATCH v2] platform/x86: int3472: Fix double free of GPIO device during unregister

[Hans de Goede]

Hi,

On 28-Oct-25 7:30 AM, Qiu Wenbo wrote:
> From: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
> 
> regulator_unregister() already frees the associated GPIO device. On
> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
> random failures when other drivers (typically Intel THC) attempt to
> allocate interrupts. The root cause is that the reference count of the
> pinctrl_intel_platform module unexpectedly drops to zero when this
> driver defers its probe.
> 
> This behavior can also be reproduced by unloading the module directly.
> 
> Fix the issue by removing the redundant release of the GPIO device
> during regulator unregistration.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")
> Signed-off-by: Qiu Wenbo <qiuwenbo@kylinsec.com.cn>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans



> ---
> Changes since V1:
>  - Add Reviewed-by: from Andy Shevchenko and Sakari Ailus
>  - Add Cc to stable@
>  - Remove the blank line after Fixes:
> 
>  drivers/platform/x86/intel/int3472/clk_and_regulator.c | 5 +----
>  include/linux/platform_data/x86/int3472.h              | 1 -
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> index 476ec24d37020..9e052b164a1ab 100644
> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> @@ -245,15 +245,12 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>  	if (IS_ERR(regulator->rdev))
>  		return PTR_ERR(regulator->rdev);
>  
> -	int3472->regulators[int3472->n_regulator_gpios].ena_gpio = gpio;
>  	int3472->n_regulator_gpios++;
>  	return 0;
>  }
>  
>  void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472)
>  {
> -	for (int i = 0; i < int3472->n_regulator_gpios; i++) {
> +	for (int i = 0; i < int3472->n_regulator_gpios; i++)
>  		regulator_unregister(int3472->regulators[i].rdev);
> -		gpiod_put(int3472->regulators[i].ena_gpio);
> -	}
>  }
> diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
> index 1571e9157fa50..b1b837583d544 100644
> --- a/include/linux/platform_data/x86/int3472.h
> +++ b/include/linux/platform_data/x86/int3472.h
> @@ -100,7 +100,6 @@ struct int3472_gpio_regulator {
>  	struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT * 2];
>  	char supply_name_upper[GPIO_SUPPLY_NAME_LENGTH];
>  	char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
> -	struct gpio_desc *ena_gpio;
>  	struct regulator_dev *rdev;
>  	struct regulator_desc rdesc;
>  };

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Andy Shevchenko]

On Tue, Oct 28, 2025 at 08:55:07AM +0000, Dan Scally wrote:
> On 24/10/2025 06:05, Qiu Wenbo wrote:
> > 
> > regulator_unregister() already frees the associated GPIO device. On
> > ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
> > random failures when other drivers (typically Intel THC) attempt to
> > allocate interrupts. The root cause is that the reference count of the
> > pinctrl_intel_platform module unexpectedly drops to zero when this
> > driver defers its probe.
> > 
> > This behavior can also be reproduced by unloading the module directly.
> > 
> > Fix the issue by removing the redundant release of the GPIO device
> > during regulator unregistration.
> > 
> > Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")

> However the Fixes tag I wonder about; devm_gpiod_get() will also result in a
> call to gpiod_put() when the module is unloaded; doesn't that mean that the
> same issue  will occur before that commit?

Actually a good question! To me sounds like it's a bug(?) in regulator code.
It must not release resources it didn't acquire. This sounds like a clear
layering violation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Hans de Goede]

Hi,

On 28-Oct-25 11:02 AM, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 08:55:07AM +0000, Dan Scally wrote:
>> On 24/10/2025 06:05, Qiu Wenbo wrote:
>>>
>>> regulator_unregister() already frees the associated GPIO device. On
>>> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
>>> random failures when other drivers (typically Intel THC) attempt to
>>> allocate interrupts. The root cause is that the reference count of the
>>> pinctrl_intel_platform module unexpectedly drops to zero when this
>>> driver defers its probe.
>>>
>>> This behavior can also be reproduced by unloading the module directly.
>>>
>>> Fix the issue by removing the redundant release of the GPIO device
>>> during regulator unregistration.
>>>
>>> Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")
> 
>> However the Fixes tag I wonder about; devm_gpiod_get() will also result in a
>> call to gpiod_put() when the module is unloaded; doesn't that mean that the
>> same issue  will occur before that commit?
> 
> Actually a good question! To me sounds like it's a bug(?) in regulator code.
> It must not release resources it didn't acquire. This sounds like a clear
> layering violation.

I think the problem is that when it comes from devicetree it is acquired
by the regulator core. Only when passed as platform-data as we do here does
this layering violation occur.

I do believe that a transfer of ownership ad done here is ok for
the platform-data special case.

Regards,

Hans

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Andy Shevchenko]

On Tue, Oct 28, 2025 at 11:38:00AM +0100, Hans de Goede wrote:
> On 28-Oct-25 11:02 AM, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 08:55:07AM +0000, Dan Scally wrote:
> >> On 24/10/2025 06:05, Qiu Wenbo wrote:
> >>>
> >>> regulator_unregister() already frees the associated GPIO device. On
> >>> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
> >>> random failures when other drivers (typically Intel THC) attempt to
> >>> allocate interrupts. The root cause is that the reference count of the
> >>> pinctrl_intel_platform module unexpectedly drops to zero when this
> >>> driver defers its probe.
> >>>
> >>> This behavior can also be reproduced by unloading the module directly.
> >>>
> >>> Fix the issue by removing the redundant release of the GPIO device
> >>> during regulator unregistration.
> >>>
> >>> Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")
> > 
> >> However the Fixes tag I wonder about; devm_gpiod_get() will also result in a
> >> call to gpiod_put() when the module is unloaded; doesn't that mean that the
> >> same issue  will occur before that commit?
> > 
> > Actually a good question! To me sounds like it's a bug(?) in regulator code.
> > It must not release resources it didn't acquire. This sounds like a clear
> > layering violation.
> 
> I think the problem is that when it comes from devicetree it is acquired
> by the regulator core.

Hmm... I probably missed that, but I failed to see this. Any pointers?

> Only when passed as platform-data as we do here does
> this layering violation occur.
> 
> I do believe that a transfer of ownership ad done here is ok for
> the platform-data special case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Dan Scally]

On 28/10/2025 10:54, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 11:38:00AM +0100, Hans de Goede wrote:
>> On 28-Oct-25 11:02 AM, Andy Shevchenko wrote:
>>> On Tue, Oct 28, 2025 at 08:55:07AM +0000, Dan Scally wrote:
>>>> On 24/10/2025 06:05, Qiu Wenbo wrote:
>>>>>
>>>>> regulator_unregister() already frees the associated GPIO device. On
>>>>> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
>>>>> random failures when other drivers (typically Intel THC) attempt to
>>>>> allocate interrupts. The root cause is that the reference count of the
>>>>> pinctrl_intel_platform module unexpectedly drops to zero when this
>>>>> driver defers its probe.
>>>>>
>>>>> This behavior can also be reproduced by unloading the module directly.
>>>>>
>>>>> Fix the issue by removing the redundant release of the GPIO device
>>>>> during regulator unregistration.
>>>>>
>>>>> Fixes: 1e5d088a52c2 ("platform/x86: int3472: Stop using devm_gpiod_get()")
>>>
>>>> However the Fixes tag I wonder about; devm_gpiod_get() will also result in a
>>>> call to gpiod_put() when the module is unloaded; doesn't that mean that the
>>>> same issue  will occur before that commit?
>>>
>>> Actually a good question! To me sounds like it's a bug(?) in regulator code.
>>> It must not release resources it didn't acquire. This sounds like a clear
>>> layering violation.
>>
>> I think the problem is that when it comes from devicetree it is acquired
>> by the regulator core.
> 
> Hmm... I probably missed that, but I failed to see this. Any pointers?

They can come through the struct regulator_desc.of_parse_cb(), which is called in
regulator_of_init_data(), from regulator_register(). For example: 
https://elixir.bootlin.com/linux/v6.17.5/source/drivers/power/supply/mt6370-charger.c#L234>
>> Only when passed as platform-data as we do here does
>> this layering violation occur.
>>
>> I do believe that a transfer of ownership ad done here is ok for
>> the platform-data special case.
> 

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Andy Shevchenko]

On Tue, Oct 28, 2025 at 11:09:12AM +0000, Dan Scally wrote:
> On 28/10/2025 10:54, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 11:38:00AM +0100, Hans de Goede wrote:
> > > On 28-Oct-25 11:02 AM, Andy Shevchenko wrote:
> > > > On Tue, Oct 28, 2025 at 08:55:07AM +0000, Dan Scally wrote:
> > > > > On 24/10/2025 06:05, Qiu Wenbo wrote:

...

> > > > > However the Fixes tag I wonder about; devm_gpiod_get() will also result in a
> > > > > call to gpiod_put() when the module is unloaded; doesn't that mean that the
> > > > > same issue  will occur before that commit?
> > > > 
> > > > Actually a good question! To me sounds like it's a bug(?) in regulator code.
> > > > It must not release resources it didn't acquire. This sounds like a clear
> > > > layering violation.
> > > 
> > > I think the problem is that when it comes from devicetree it is acquired
> > > by the regulator core.
> > 
> > Hmm... I probably missed that, but I failed to see this. Any pointers?
> 
> They can come through the struct regulator_desc.of_parse_cb(), which is called in
> regulator_of_init_data(), from regulator_register(). For example: https://elixir.bootlin.com/linux/v6.17.5/source/drivers/power/supply/mt6370-charger.c#L234>

Ah, thank you, Dan, for the pointers. Indeed, that's how it's done. Hmm, still
why can't we let the regulator consumer to decide when to clean the resource?
I think this is an attempt to have a refcounting against shared GPIO resource
and it should be done in the GPIOLIB (if not yet). In regulator that put
call should probably be conditional (based on the source of GPIO request).

> > > Only when passed as platform-data as we do here does
> > > this layering violation occur.
> > > 
> > > I do believe that a transfer of ownership ad done here is ok for
> > > the platform-data special case.

As an exception, maybe...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Hans de Goede]

Hi,

On 28-Oct-25 12:38 PM, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 11:09:12AM +0000, Dan Scally wrote:
>> On 28/10/2025 10:54, Andy Shevchenko wrote:
>>> On Tue, Oct 28, 2025 at 11:38:00AM +0100, Hans de Goede wrote:
>>>> On 28-Oct-25 11:02 AM, Andy Shevchenko wrote:
>>>>> On Tue, Oct 28, 2025 at 08:55:07AM +0000, Dan Scally wrote:
>>>>>> On 24/10/2025 06:05, Qiu Wenbo wrote:
> 
> ...
> 
>>>>>> However the Fixes tag I wonder about; devm_gpiod_get() will also result in a
>>>>>> call to gpiod_put() when the module is unloaded; doesn't that mean that the
>>>>>> same issue  will occur before that commit?
>>>>>
>>>>> Actually a good question! To me sounds like it's a bug(?) in regulator code.
>>>>> It must not release resources it didn't acquire. This sounds like a clear
>>>>> layering violation.
>>>>
>>>> I think the problem is that when it comes from devicetree it is acquired
>>>> by the regulator core.
>>>
>>> Hmm... I probably missed that, but I failed to see this. Any pointers?
>>
>> They can come through the struct regulator_desc.of_parse_cb(), which is called in
>> regulator_of_init_data(), from regulator_register(). For example: https://elixir.bootlin.com/linux/v6.17.5/source/drivers/power/supply/mt6370-charger.c#L234>
> 
> Ah, thank you, Dan, for the pointers. Indeed, that's how it's done. Hmm, still
> why can't we let the regulator consumer to decide when to clean the resource?
> I think this is an attempt to have a refcounting against shared GPIO resource
> and it should be done in the GPIOLIB (if not yet). In regulator that put
> call should probably be conditional (based on the source of GPIO request).

Fixing this sounds like a somewhat big undertaking. In the mean time
I think we should move forward with this patch to fix the immediate
issue with the double free.

Regards,

Hans

Re: [PATCH] platform/x86: int3472: Fix double free of GPIO device during unregister

[Andy Shevchenko]

On Tue, Oct 28, 2025 at 03:36:56PM +0100, Hans de Goede wrote:
> On 28-Oct-25 12:38 PM, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 11:09:12AM +0000, Dan Scally wrote:
> >> On 28/10/2025 10:54, Andy Shevchenko wrote:
> >>> On Tue, Oct 28, 2025 at 11:38:00AM +0100, Hans de Goede wrote:
> >>>> On 28-Oct-25 11:02 AM, Andy Shevchenko wrote:
> >>>>> On Tue, Oct 28, 2025 at 08:55:07AM +0000, Dan Scally wrote:
> >>>>>> On 24/10/2025 06:05, Qiu Wenbo wrote:

...

> >>>>>> However the Fixes tag I wonder about; devm_gpiod_get() will also result in a
> >>>>>> call to gpiod_put() when the module is unloaded; doesn't that mean that the
> >>>>>> same issue  will occur before that commit?
> >>>>>
> >>>>> Actually a good question! To me sounds like it's a bug(?) in regulator code.
> >>>>> It must not release resources it didn't acquire. This sounds like a clear
> >>>>> layering violation.
> >>>>
> >>>> I think the problem is that when it comes from devicetree it is acquired
> >>>> by the regulator core.
> >>>
> >>> Hmm... I probably missed that, but I failed to see this. Any pointers?
> >>
> >> They can come through the struct regulator_desc.of_parse_cb(), which is called in
> >> regulator_of_init_data(), from regulator_register(). For example: https://elixir.bootlin.com/linux/v6.17.5/source/drivers/power/supply/mt6370-charger.c#L234>
> > 
> > Ah, thank you, Dan, for the pointers. Indeed, that's how it's done. Hmm, still
> > why can't we let the regulator consumer to decide when to clean the resource?
> > I think this is an attempt to have a refcounting against shared GPIO resource
> > and it should be done in the GPIOLIB (if not yet). In regulator that put
> > call should probably be conditional (based on the source of GPIO request).
> 
> Fixing this sounds like a somewhat big undertaking. In the mean time
> I think we should move forward with this patch to fix the immediate
> issue with the double free.

And I am not objecting to apply this as you may see in the tags given so far.
In regard to the undertaking it seems in Bart's (GPIOLIB maintainer) TODO list.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] platform/x86: int3472: Fix double free of GPIO device during unregister

[Ilpo Järvinen]

On Tue, 28 Oct 2025 14:30:09 +0800, Qiu Wenbo wrote:

> regulator_unregister() already frees the associated GPIO device. On
> ThinkPad X9 (Lunar Lake), this causes a double free issue that leads to
> random failures when other drivers (typically Intel THC) attempt to
> allocate interrupts. The root cause is that the reference count of the
> pinctrl_intel_platform module unexpectedly drops to zero when this
> driver defers its probe.
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo-fixes branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-fixes branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86: int3472: Fix double free of GPIO device during unregister
      commit: f0f7a3f542c1698edb69075f25a3f846207facba

--
 i.