* [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access
@ 2025-02-11 12:44 Manaf Meethalavalappu Pallikunhi
2025-02-12 19:42 ` Rafael J. Wysocki
0 siblings, 1 reply; 3+ messages in thread
From: Manaf Meethalavalappu Pallikunhi @ 2025-02-11 12:44 UTC (permalink / raw)
To: Rafael J . Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba
Cc: linux-pm, linux-kernel, Manaf Meethalavalappu Pallikunhi
Currently, the thermal zone sysfs is created before setting the
governor for that thermal zone during registration. If a thermal
zone is being registered while a userspace module tries to access
the same thermal zone policy sysfs node, it can lead to a potential
NULL pointer dereference issue in the policy sysfs path.
To avoid this race condition, set the thermal zone governor first
before enabling the thermal zone sysfs during registration.
This change fixes below issue,
[ 20.964589] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 21.049645] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
[ 21.049647] pc : policy_show+0x1c/0x3c
[ 21.049652] lr : dev_attr_show+0x38/0x7c
[ 21.049655] sp : ffffffc09a98bbf0
[ 21.049657] x29: ffffffc09a98bbf0 x28: ffffff885b940000 x27: 0000000000000000
[ 21.049660] x26: 0000000000000000 x25: 000000007ffff001 x24: 0000000000000001
[ 21.049664] x23: ffffffdca6334c78 x22: ffffff88a2b2fe00 x21: ffffff881cee8000
[ 21.049667] x20: ffffff8868318018 x19: ffffffdca7640d78 x18: ffffffdca74d94c0
[ 21.049670] x17: 00000000ae84bcd4 x16: 00000000ae84bcd4 x15: 000000002df29963
[ 21.049673] x14: 00000000cbef29c7 x13: 000000004e61db0a x12: ffffff885b940be0
[ 21.049677] x11: ffffff881cee8000 x10: 0000000000000000 x9 : ffffffdca59f00b8
[ 21.049680] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
[ 21.049683] x5 : 0000000000000040 x4 : 0000000000000000 x3 : 0000000000000004
[ 21.049686] x2 : ffffff881cee8000 x1 : ffffffdca66e5bfb x0 : ffffff881cee8000
[ 21.049689] Call trace:
[ 21.049690] policy_show+0x1c/0x3c
[ 21.049692] dev_attr_show+0x38/0x7c
[ 21.049695] sysfs_kf_seq_show+0xd8/0x160
[ 21.049699] kernfs_seq_show+0x44/0x54
[ 21.049701] seq_read_iter+0x16c/0x4ec
[ 21.049705] kernfs_fop_read_iter+0x64/0x1d8
[ 21.049709] vfs_read+0x2d8/0x33c
[ 21.049711] ksys_read+0x78/0xe8
[ 21.049714] __arm64_sys_read+0x1c/0x2c
[ 21.049716] invoke_syscall+0x58/0x10c
[ 21.049719] el0_svc_common+0xa8/0xdc
[ 21.049722] do_el0_svc+0x1c/0x28
[ 21.049724] el0_svc+0x40/0x90
[ 21.049726] el0t_64_sync_handler+0x70/0xbc
[ 21.049728] el0t_64_sync+0x1a8/0x1ac
[ 21.049731] Code: f9435008 aa0203e0 d00054e1 912fec21 (f9400108)
Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
---
drivers/thermal/thermal_core.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 2328ac0d8561..c6e6b229cc6e 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1589,6 +1589,11 @@ thermal_zone_device_register_with_trips(const char *type,
tz->state = TZ_STATE_FLAG_INIT;
+ thermal_zone_device_init(tz);
+ result = thermal_zone_init_governor(tz);
+ if (result)
+ goto unregister;
+
/* sys I/F */
/* Add nodes that are always present via .groups */
result = thermal_zone_create_device_groups(tz);
@@ -1600,19 +1605,14 @@ thermal_zone_device_register_with_trips(const char *type,
thermal_zone_destroy_device_groups(tz);
goto remove_id;
}
- thermal_zone_device_init(tz);
result = device_register(&tz->device);
if (result)
goto release_device;
- result = thermal_zone_init_governor(tz);
- if (result)
- goto unregister;
-
if (!tz->tzp || !tz->tzp->no_hwmon) {
result = thermal_add_hwmon_sysfs(tz);
if (result)
- goto unregister;
+ goto release_device;
}
result = thermal_thresholds_init(tz);
@@ -1629,12 +1629,12 @@ thermal_zone_device_register_with_trips(const char *type,
remove_hwmon:
thermal_remove_hwmon_sysfs(tz);
-unregister:
- device_del(&tz->device);
release_device:
put_device(&tz->device);
remove_id:
ida_free(&thermal_tz_ida, id);
+unregister:
+ device_del(&tz->device);
free_tzp:
kfree(tz->tzp);
free_tz:
--
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access
2025-02-11 12:44 [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access Manaf Meethalavalappu Pallikunhi
@ 2025-02-12 19:42 ` Rafael J. Wysocki
2025-02-12 21:09 ` Manaf Meethalavalappu Pallikunhi
0 siblings, 1 reply; 3+ messages in thread
From: Rafael J. Wysocki @ 2025-02-12 19:42 UTC (permalink / raw)
To: Manaf Meethalavalappu Pallikunhi
Cc: Rafael J . Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
linux-pm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4754 bytes --]
On Tue, Feb 11, 2025 at 1:45 PM Manaf Meethalavalappu Pallikunhi
<quic_manafm@quicinc.com> wrote:
>
> Currently, the thermal zone sysfs is created before setting the
> governor for that thermal zone during registration. If a thermal
> zone is being registered while a userspace module tries to access
> the same thermal zone policy sysfs node, it can lead to a potential
> NULL pointer dereference issue in the policy sysfs path.
>
> To avoid this race condition, set the thermal zone governor first
> before enabling the thermal zone sysfs during registration.
> This change fixes below issue,
>
> [ 20.964589] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [ 21.049645] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
> [ 21.049647] pc : policy_show+0x1c/0x3c
> [ 21.049652] lr : dev_attr_show+0x38/0x7c
> [ 21.049655] sp : ffffffc09a98bbf0
> [ 21.049657] x29: ffffffc09a98bbf0 x28: ffffff885b940000 x27: 0000000000000000
> [ 21.049660] x26: 0000000000000000 x25: 000000007ffff001 x24: 0000000000000001
> [ 21.049664] x23: ffffffdca6334c78 x22: ffffff88a2b2fe00 x21: ffffff881cee8000
> [ 21.049667] x20: ffffff8868318018 x19: ffffffdca7640d78 x18: ffffffdca74d94c0
> [ 21.049670] x17: 00000000ae84bcd4 x16: 00000000ae84bcd4 x15: 000000002df29963
> [ 21.049673] x14: 00000000cbef29c7 x13: 000000004e61db0a x12: ffffff885b940be0
> [ 21.049677] x11: ffffff881cee8000 x10: 0000000000000000 x9 : ffffffdca59f00b8
> [ 21.049680] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
> [ 21.049683] x5 : 0000000000000040 x4 : 0000000000000000 x3 : 0000000000000004
> [ 21.049686] x2 : ffffff881cee8000 x1 : ffffffdca66e5bfb x0 : ffffff881cee8000
> [ 21.049689] Call trace:
> [ 21.049690] policy_show+0x1c/0x3c
> [ 21.049692] dev_attr_show+0x38/0x7c
> [ 21.049695] sysfs_kf_seq_show+0xd8/0x160
> [ 21.049699] kernfs_seq_show+0x44/0x54
> [ 21.049701] seq_read_iter+0x16c/0x4ec
> [ 21.049705] kernfs_fop_read_iter+0x64/0x1d8
> [ 21.049709] vfs_read+0x2d8/0x33c
> [ 21.049711] ksys_read+0x78/0xe8
> [ 21.049714] __arm64_sys_read+0x1c/0x2c
> [ 21.049716] invoke_syscall+0x58/0x10c
> [ 21.049719] el0_svc_common+0xa8/0xdc
> [ 21.049722] do_el0_svc+0x1c/0x28
> [ 21.049724] el0_svc+0x40/0x90
> [ 21.049726] el0t_64_sync_handler+0x70/0xbc
> [ 21.049728] el0t_64_sync+0x1a8/0x1ac
> [ 21.049731] Code: f9435008 aa0203e0 d00054e1 912fec21 (f9400108)
>
> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
> ---
> drivers/thermal/thermal_core.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 2328ac0d8561..c6e6b229cc6e 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1589,6 +1589,11 @@ thermal_zone_device_register_with_trips(const char *type,
>
> tz->state = TZ_STATE_FLAG_INIT;
>
> + thermal_zone_device_init(tz);
> + result = thermal_zone_init_governor(tz);
> + if (result)
> + goto unregister;
> +
> /* sys I/F */
> /* Add nodes that are always present via .groups */
> result = thermal_zone_create_device_groups(tz);
> @@ -1600,19 +1605,14 @@ thermal_zone_device_register_with_trips(const char *type,
> thermal_zone_destroy_device_groups(tz);
> goto remove_id;
> }
> - thermal_zone_device_init(tz);
> result = device_register(&tz->device);
> if (result)
> goto release_device;
>
> - result = thermal_zone_init_governor(tz);
> - if (result)
> - goto unregister;
> -
> if (!tz->tzp || !tz->tzp->no_hwmon) {
> result = thermal_add_hwmon_sysfs(tz);
> if (result)
> - goto unregister;
> + goto release_device;
> }
>
> result = thermal_thresholds_init(tz);
> @@ -1629,12 +1629,12 @@ thermal_zone_device_register_with_trips(const char *type,
>
> remove_hwmon:
> thermal_remove_hwmon_sysfs(tz);
> -unregister:
> - device_del(&tz->device);
> release_device:
> put_device(&tz->device);
> remove_id:
> ida_free(&thermal_tz_ida, id);
> +unregister:
> + device_del(&tz->device);
> free_tzp:
> kfree(tz->tzp);
> free_tz:
> --
The catch is good, but the patch isn't AFAICS. The changes in the
error path don't look correct to me in particular.
I'd rather make the attached change, please let me know if it works for you.
[-- Attachment #2: thermal-core-gov-sysfs.patch --]
[-- Type: text/x-patch, Size: 1505 bytes --]
---
drivers/thermal/thermal_core.c | 2 +-
drivers/thermal/thermal_core.h | 1 +
drivers/thermal/thermal_sysfs.c | 7 ++++++-
3 files changed, 8 insertions(+), 2 deletions(-)
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -353,7 +353,7 @@
thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
}
-static struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz)
+struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz)
{
if (tz->governor)
return tz->governor;
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -262,6 +262,7 @@
void __thermal_zone_device_update(struct thermal_zone_device *tz,
enum thermal_notify_event event);
void thermal_zone_device_critical_reboot(struct thermal_zone_device *tz);
+struct thermal_governor *thermal_get_tz_governor(struct thermal_zone_device *tz);
void thermal_governor_update_tz(struct thermal_zone_device *tz,
enum thermal_notify_event reason);
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -209,8 +209,13 @@
policy_show(struct device *dev, struct device_attribute *devattr, char *buf)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
+ struct thermal_governor *governor;
- return sprintf(buf, "%s\n", tz->governor->name);
+ guard(thermal_zone)(tz);
+
+ governor = thermal_get_tz_governor(tz);
+
+ return sprintf(buf, "%s\n", governor->name);
}
static ssize_t
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access
2025-02-12 19:42 ` Rafael J. Wysocki
@ 2025-02-12 21:09 ` Manaf Meethalavalappu Pallikunhi
0 siblings, 0 replies; 3+ messages in thread
From: Manaf Meethalavalappu Pallikunhi @ 2025-02-12 21:09 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Daniel Lezcano, Zhang Rui, Lukasz Luba, linux-pm, linux-kernel
Hi Rafael,
Thank you for reviewing the patch.
On 2/13/2025 1:12 AM, Rafael J. Wysocki wrote:
> On Tue, Feb 11, 2025 at 1:45 PM Manaf Meethalavalappu Pallikunhi
> <quic_manafm@quicinc.com> wrote:
>> Currently, the thermal zone sysfs is created before setting the
>> governor for that thermal zone during registration. If a thermal
>> zone is being registered while a userspace module tries to access
>> the same thermal zone policy sysfs node, it can lead to a potential
>> NULL pointer dereference issue in the policy sysfs path.
>>
>> To avoid this race condition, set the thermal zone governor first
>> before enabling the thermal zone sysfs during registration.
>> This change fixes below issue,
>>
>> [ 20.964589] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>> [ 21.049645] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
>> [ 21.049647] pc : policy_show+0x1c/0x3c
>> [ 21.049652] lr : dev_attr_show+0x38/0x7c
>> [ 21.049655] sp : ffffffc09a98bbf0
>> [ 21.049657] x29: ffffffc09a98bbf0 x28: ffffff885b940000 x27: 0000000000000000
>> [ 21.049660] x26: 0000000000000000 x25: 000000007ffff001 x24: 0000000000000001
>> [ 21.049664] x23: ffffffdca6334c78 x22: ffffff88a2b2fe00 x21: ffffff881cee8000
>> [ 21.049667] x20: ffffff8868318018 x19: ffffffdca7640d78 x18: ffffffdca74d94c0
>> [ 21.049670] x17: 00000000ae84bcd4 x16: 00000000ae84bcd4 x15: 000000002df29963
>> [ 21.049673] x14: 00000000cbef29c7 x13: 000000004e61db0a x12: ffffff885b940be0
>> [ 21.049677] x11: ffffff881cee8000 x10: 0000000000000000 x9 : ffffffdca59f00b8
>> [ 21.049680] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
>> [ 21.049683] x5 : 0000000000000040 x4 : 0000000000000000 x3 : 0000000000000004
>> [ 21.049686] x2 : ffffff881cee8000 x1 : ffffffdca66e5bfb x0 : ffffff881cee8000
>> [ 21.049689] Call trace:
>> [ 21.049690] policy_show+0x1c/0x3c
>> [ 21.049692] dev_attr_show+0x38/0x7c
>> [ 21.049695] sysfs_kf_seq_show+0xd8/0x160
>> [ 21.049699] kernfs_seq_show+0x44/0x54
>> [ 21.049701] seq_read_iter+0x16c/0x4ec
>> [ 21.049705] kernfs_fop_read_iter+0x64/0x1d8
>> [ 21.049709] vfs_read+0x2d8/0x33c
>> [ 21.049711] ksys_read+0x78/0xe8
>> [ 21.049714] __arm64_sys_read+0x1c/0x2c
>> [ 21.049716] invoke_syscall+0x58/0x10c
>> [ 21.049719] el0_svc_common+0xa8/0xdc
>> [ 21.049722] do_el0_svc+0x1c/0x28
>> [ 21.049724] el0_svc+0x40/0x90
>> [ 21.049726] el0t_64_sync_handler+0x70/0xbc
>> [ 21.049728] el0t_64_sync+0x1a8/0x1ac
>> [ 21.049731] Code: f9435008 aa0203e0 d00054e1 912fec21 (f9400108)
>>
>> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com>
>> ---
>> drivers/thermal/thermal_core.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 2328ac0d8561..c6e6b229cc6e 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1589,6 +1589,11 @@ thermal_zone_device_register_with_trips(const char *type,
>>
>> tz->state = TZ_STATE_FLAG_INIT;
>>
>> + thermal_zone_device_init(tz);
>> + result = thermal_zone_init_governor(tz);
>> + if (result)
>> + goto unregister;
>> +
>> /* sys I/F */
>> /* Add nodes that are always present via .groups */
>> result = thermal_zone_create_device_groups(tz);
>> @@ -1600,19 +1605,14 @@ thermal_zone_device_register_with_trips(const char *type,
>> thermal_zone_destroy_device_groups(tz);
>> goto remove_id;
>> }
>> - thermal_zone_device_init(tz);
>> result = device_register(&tz->device);
>> if (result)
>> goto release_device;
>>
>> - result = thermal_zone_init_governor(tz);
>> - if (result)
>> - goto unregister;
>> -
>> if (!tz->tzp || !tz->tzp->no_hwmon) {
>> result = thermal_add_hwmon_sysfs(tz);
>> if (result)
>> - goto unregister;
>> + goto release_device;
>> }
>>
>> result = thermal_thresholds_init(tz);
>> @@ -1629,12 +1629,12 @@ thermal_zone_device_register_with_trips(const char *type,
>>
>> remove_hwmon:
>> thermal_remove_hwmon_sysfs(tz);
>> -unregister:
>> - device_del(&tz->device);
>> release_device:
>> put_device(&tz->device);
>> remove_id:
>> ida_free(&thermal_tz_ida, id);
>> +unregister:
>> + device_del(&tz->device);
>> free_tzp:
>> kfree(tz->tzp);
>> free_tz:
>> --
> The catch is good, but the patch isn't AFAICS. The changes in the
> error path don't look correct to me in particular.
I understood the issues in the error path. I’ll try to fix them in the
v2 patch. Is there any concern with setting the governor before
initializing sysfs ?
>
> I'd rather make the attached change, please let me know if it works for you.
Yes, It will work for current issue. But it could provide a incorrect
policy information to userspace if a thermal zone is created with a
governor other than the default one (def_governor), right ?
Thanks,
Manaf
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-02-12 21:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-11 12:44 [PATCH] thermal: core: Fix race between zone registration and userspace sysfs access Manaf Meethalavalappu Pallikunhi
2025-02-12 19:42 ` Rafael J. Wysocki
2025-02-12 21:09 ` Manaf Meethalavalappu Pallikunhi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®