When offlining all cores on a CPU, the following system panic
occurs:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: strlen+0x0/0x20
<snip>
Call Trace:
? kernfs_name_hash+0x17/0x80
kernfs_find_ns+0x3f/0xd0
kernfs_remove_by_name_ns+0x36/0xa0
remove_files.isra.1+0x36/0x70
sysfs_remove_group+0x44/0x90
sysfs_remove_groups+0x2e/0x50
device_remove_attrs+0x5e/0x90
device_del+0x1ea/0x350
device_unregister+0x1a/0x60
thermal_zone_device_unregister+0x1f2/0x210
pkg_thermal_cpu_offline+0x14f/0x1a0 [x86_pkg_temp_thermal]
? kzalloc.constprop.2+0x10/0x10 [x86_pkg_temp_thermal]
cpuhp_invoke_callback+0x8d/0x3f0
cpuhp_down_callbacks+0x42/0x80
cpuhp_thread_fun+0x8b/0xf0
smpboot_thread_fn+0x110/0x160
kthread+0x101/0x140
? sort_range+0x30/0x30
? kthread_park+0x90/0x90
ret_from_fork+0x25/0x30
thermal_zone_create_device_group() sets attribute_groups in
thermal_zone_attribute_groups[] to tz->device.groups. But these
attributes_groups do not have name argument.
So when offlining all cores on CPU and executing
thermal_zone_device_unregister(), the panic occurs in strlen()
called from kernfs_name_hash() because name argument is NULL.
The patch adds thermal_zone_remove_device_groups() to free
tz->device.groups and set NULL pointer.
Signed-off-by: Yasuaki Ishimatsu <[email protected]>
CC: Zhang Rui <[email protected]>
CC: Eduardo Valentin <[email protected]>
---
drivers/thermal/thermal_core.c | 3 ++-
drivers/thermal/thermal_core.h | 1 +
drivers/thermal/thermal_sysfs.c | 6 ++++++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 641faab..926e385 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1251,6 +1251,7 @@ struct thermal_zone_device *
unregister:
release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
+ thermal_zone_remove_device_groups(tz);
device_unregister(&tz->device);
return ERR_PTR(result);
}
@@ -1315,8 +1316,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
idr_destroy(&tz->idr);
mutex_destroy(&tz->lock);
+ thermal_zone_remove_device_groups(tz);
device_unregister(&tz->device);
- kfree(tz->device.groups);
}
EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 2412b37..e3a60db 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -70,6 +70,7 @@ void thermal_zone_device_unbind_exception(struct thermal_zone_device *,
int thermal_build_list_of_policies(char *buf);
/* sysfs I/F */
+void thermal_zone_remove_device_groups(struct thermal_zone_device *tz);
int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
/* used only at binding time */
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index a694de9..3dfd29b 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -605,6 +605,12 @@ static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
return 0;
}
+void thermal_zone_remove_device_groups(struct thermal_zone_device *tz)
+{
+ kfree(tz->device.groups);
+ tz->device.groups = NULL;
+}
+
int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
int mask)
{
--
1.8.3.1
Hey Yasuaki San,
On Thu, Dec 15, 2016 at 04:47:08PM -0500, Yasuaki Ishimatsu wrote:
> When offlining all cores on a CPU, the following system panic
> occurs:
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: strlen+0x0/0x20
> <snip>
> Call Trace:
> ? kernfs_name_hash+0x17/0x80
> kernfs_find_ns+0x3f/0xd0
> kernfs_remove_by_name_ns+0x36/0xa0
> remove_files.isra.1+0x36/0x70
> sysfs_remove_group+0x44/0x90
> sysfs_remove_groups+0x2e/0x50
> device_remove_attrs+0x5e/0x90
> device_del+0x1ea/0x350
> device_unregister+0x1a/0x60
> thermal_zone_device_unregister+0x1f2/0x210
> pkg_thermal_cpu_offline+0x14f/0x1a0 [x86_pkg_temp_thermal]
> ? kzalloc.constprop.2+0x10/0x10 [x86_pkg_temp_thermal]
> cpuhp_invoke_callback+0x8d/0x3f0
> cpuhp_down_callbacks+0x42/0x80
> cpuhp_thread_fun+0x8b/0xf0
> smpboot_thread_fn+0x110/0x160
> kthread+0x101/0x140
> ? sort_range+0x30/0x30
> ? kthread_park+0x90/0x90
> ret_from_fork+0x25/0x30
>
> thermal_zone_create_device_group() sets attribute_groups in
> thermal_zone_attribute_groups[] to tz->device.groups. But these
> attributes_groups do not have name argument.
>
> So when offlining all cores on CPU and executing
> thermal_zone_device_unregister(), the panic occurs in strlen()
> called from kernfs_name_hash() because name argument is NULL.
>
> The patch adds thermal_zone_remove_device_groups() to free
> tz->device.groups and set NULL pointer.
>
Thanks for finding this and sending a fix. Overall I am OK with the
patch, but I have some questions, as follows.
> Signed-off-by: Yasuaki Ishimatsu <[email protected]>
> CC: Zhang Rui <[email protected]>
> CC: Eduardo Valentin <[email protected]>
> ---
> drivers/thermal/thermal_core.c | 3 ++-
> drivers/thermal/thermal_core.h | 1 +
> drivers/thermal/thermal_sysfs.c | 6 ++++++
First question is, are you seeing same problem with cooling devices ?
They follow same strategy, of setting the groups property, and the
cooling device groups also have no name.
> 3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 641faab..926e385 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1251,6 +1251,7 @@ struct thermal_zone_device *
>
> unregister:
> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> + thermal_zone_remove_device_groups(tz);
> device_unregister(&tz->device);
> return ERR_PTR(result);
> }
> @@ -1315,8 +1316,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
> idr_destroy(&tz->idr);
> mutex_destroy(&tz->lock);
> + thermal_zone_remove_device_groups(tz);
> device_unregister(&tz->device);
> - kfree(tz->device.groups);
> }
> EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
>
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 2412b37..e3a60db 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -70,6 +70,7 @@ void thermal_zone_device_unbind_exception(struct thermal_zone_device *,
> int thermal_build_list_of_policies(char *buf);
>
> /* sysfs I/F */
> +void thermal_zone_remove_device_groups(struct thermal_zone_device *tz);
> int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
> void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
> /* used only at binding time */
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index a694de9..3dfd29b 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -605,6 +605,12 @@ static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
> return 0;
> }
>
> +void thermal_zone_remove_device_groups(struct thermal_zone_device *tz)
> +{
> + kfree(tz->device.groups);
OK. This part needs to be done regardless
> + tz->device.groups = NULL;
This almost defeats the purpose of the effort to put all properties to
device.groups, as we need to care about releasing them.
I guess the problem is we cannot keep the same sysfs entries if we set a
name on the thermal device groups. That would break userspace. And if we
do not set, we need to care about releasing them.
Almost feel like we might consider patching sysfs core.
Or maybe find another way to setup the device groups for thermal
devices.
> +}
> +
> int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
> int mask)
> {
> --
> 1.8.3.1
>
On 12/19/2016 11:21 PM, Eduardo Valentin wrote:
> Hey Yasuaki San,
>
> On Thu, Dec 15, 2016 at 04:47:08PM -0500, Yasuaki Ishimatsu wrote:
>> When offlining all cores on a CPU, the following system panic
>> occurs:
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: strlen+0x0/0x20
>> <snip>
>> Call Trace:
>> ? kernfs_name_hash+0x17/0x80
>> kernfs_find_ns+0x3f/0xd0
>> kernfs_remove_by_name_ns+0x36/0xa0
>> remove_files.isra.1+0x36/0x70
>> sysfs_remove_group+0x44/0x90
>> sysfs_remove_groups+0x2e/0x50
>> device_remove_attrs+0x5e/0x90
>> device_del+0x1ea/0x350
>> device_unregister+0x1a/0x60
>> thermal_zone_device_unregister+0x1f2/0x210
>> pkg_thermal_cpu_offline+0x14f/0x1a0 [x86_pkg_temp_thermal]
>> ? kzalloc.constprop.2+0x10/0x10 [x86_pkg_temp_thermal]
>> cpuhp_invoke_callback+0x8d/0x3f0
>> cpuhp_down_callbacks+0x42/0x80
>> cpuhp_thread_fun+0x8b/0xf0
>> smpboot_thread_fn+0x110/0x160
>> kthread+0x101/0x140
>> ? sort_range+0x30/0x30
>> ? kthread_park+0x90/0x90
>> ret_from_fork+0x25/0x30
>>
>> thermal_zone_create_device_group() sets attribute_groups in
>> thermal_zone_attribute_groups[] to tz->device.groups. But these
>> attributes_groups do not have name argument.
>>
>> So when offlining all cores on CPU and executing
>> thermal_zone_device_unregister(), the panic occurs in strlen()
>> called from kernfs_name_hash() because name argument is NULL.
>>
>> The patch adds thermal_zone_remove_device_groups() to free
>> tz->device.groups and set NULL pointer.
>>
>
> Thanks for finding this and sending a fix. Overall I am OK with the
> patch, but I have some questions, as follows.
>
>> Signed-off-by: Yasuaki Ishimatsu <[email protected]>
>> CC: Zhang Rui <[email protected]>
>> CC: Eduardo Valentin <[email protected]>
>> ---
>> drivers/thermal/thermal_core.c | 3 ++-
>> drivers/thermal/thermal_core.h | 1 +
>> drivers/thermal/thermal_sysfs.c | 6 ++++++
>
> First question is, are you seeing same problem with cooling devices ?
No, I'm not.
I tested physical cpu hotplug on my server and found only the issue.
> They follow same strategy, of setting the groups property, and the
> cooling device groups also have no name.
>
>> 3 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 641faab..926e385 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1251,6 +1251,7 @@ struct thermal_zone_device *
>>
>> unregister:
>> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>> + thermal_zone_remove_device_groups(tz);
>> device_unregister(&tz->device);
>> return ERR_PTR(result);
>> }
>> @@ -1315,8 +1316,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz)
>> release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id);
>> idr_destroy(&tz->idr);
>> mutex_destroy(&tz->lock);
>> + thermal_zone_remove_device_groups(tz);
>> device_unregister(&tz->device);
>> - kfree(tz->device.groups);
>> }
>> EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
>>
>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
>> index 2412b37..e3a60db 100644
>> --- a/drivers/thermal/thermal_core.h
>> +++ b/drivers/thermal/thermal_core.h
>> @@ -70,6 +70,7 @@ void thermal_zone_device_unbind_exception(struct thermal_zone_device *,
>> int thermal_build_list_of_policies(char *buf);
>>
>> /* sysfs I/F */
>> +void thermal_zone_remove_device_groups(struct thermal_zone_device *tz);
>> int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
>> void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
>> /* used only at binding time */
>> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
>> index a694de9..3dfd29b 100644
>> --- a/drivers/thermal/thermal_sysfs.c
>> +++ b/drivers/thermal/thermal_sysfs.c
>> @@ -605,6 +605,12 @@ static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
>> return 0;
>> }
>>
>> +void thermal_zone_remove_device_groups(struct thermal_zone_device *tz)
>> +{
>> + kfree(tz->device.groups);
>
> OK. This part needs to be done regardless
>
>> + tz->device.groups = NULL;
>
> This almost defeats the purpose of the effort to put all properties to
> device.groups, as we need to care about releasing them.
tz->device.groups has been released by kfree(). So if we don't set NULL
to tz->device.groups, it is possible to touch the released memory. Therefore
tz->device.groups should be set to NULL.
Thanks,
Yasuaki Ishimatsu
>
> I guess the problem is we cannot keep the same sysfs entries if we set a
> name on the thermal device groups. That would break userspace. And if we
> do not set, we need to care about releasing them.
>
> Almost feel like we might consider patching sysfs core.
> Or maybe find another way to setup the device groups for thermal
> devices.
>
>> +}
>> +
>> int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
>> int mask)
>> {
>> --
>> 1.8.3.1
>>
>