Use get_online_cpus to avoid races involving for_each_online_cpu
PATCH 3/3 Use get_online_cpus to avoid races involving for_each_online_cpu
via_cputemp_init in drivers/hwmon/via-cputemp.c loops with for_each_online_cpu, adding platform_devices, then calls register_hotcpu_notifier. If a CPU is offlined between the loop and register_hotcpu_notifier, then later onlined, via_cputemp_device_add will attempt to platform devices with the same ID.
This fix surrounds for_each_online_cpu and register_hotcpu_notifier with get_online_cpus+put_online_cpus.
Build tested.
--- drivers/hwmon/via-cputemp.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/hwmon/via-cputemp.c +3 -0
@@ -328,6 +328,7 @@ static int __init via_cputemp_init(void)if (err) goto exit; +get_online_cpus ();for_each_online_cpu (i) {struct cpuinfo_x86 *c = &cpu_data (i);
@@ -347,12 +348,14 @@ static int __init via_cputemp_init(void)#ifndef CONFIG_HOTPLUG_CPU if (list_empty (&pdev_list)) { +put_online_cpus (); err = -ENODEV ; goto exit_driver_unreg; }#endif register_hotcpu_notifier (&via_cputemp_cpu_notifier); +put_online_cpus ();return 0 ;#ifndef CONFIG_HOTPLUG_CPU
Signature · 1 line
1.7.10.4
Hi Silas,
thanks a lot for the finding and addressing the issue.
Quoted 1 line · Silas Boyd-Wickizer
Signed-off-by Silas Boyd-Wickizer <[email protected]>
Signature · 4 lines
- Harald Welte <[email protected]> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Hi Silas,
Quoted 5 lines · Silas Boyd-Wickizer
via_cputemp_initindrivers/hwmon/via-cputemp.cloops withfor_each_online_cpu, addingplatform_devices, then callsregister_hotcpu_notifier. If a CPU is offlined between the loop andregister_hotcpu_notifier, then later onlined,via_cputemp_device_addwill attempt to platform devices with the same ID.
Missing word in this last sentence.
Quoted 5 lines
This fix surrounds
for_each_online_cpuandregister_hotcpu_notifierwith get_online_cpus+put_online_cpus.Build tested.
Thanks for reporting and for the fix. Two questions:
What about via_cputemp_exit()? While less obvious, I suspect it is racy too. The notifier is unregistered first. If a CPU gets offline before the devices are removed, we will have a device pointing to a non-existent CPU for a short time. I think we should play it safe and use get/put_online_cpus() there too, as I seem to understand it guarantees CPUs can't go offline when we wouldn't handle that event properly. Alternatively, unregistering the platform driver first might close the race.
Secondly, drivers/hwmon/coretemp.c is very similar to via-cputemp.c, so I think it needs the exact same fix(es).
Quoted 33 lines
Signed-off-by Silas Boyd-Wickizer <[email protected]>--- drivers/hwmon/via-cputemp.c | 3 +++ 1 file changed, 3 insertions(+)drivers/hwmon/via-cputemp.c +3
-0@@ -328,6 +328,7 @@ static int __init via_cputemp_init(void)if (err) goto exit; +get_online_cpus ();for_each_online_cpu (i) {struct cpuinfo_x86 *c = &cpu_data (i);@@ -347,12 +348,14 @@ static int __init via_cputemp_init(void)#ifndef CONFIG_HOTPLUG_CPU if (list_empty (&pdev_list)) { +put_online_cpus (); err = -ENODEV ; goto exit_driver_unreg; }#endif register_hotcpu_notifier (&via_cputemp_cpu_notifier); +put_online_cpus ();return 0 ;#ifndef CONFIG_HOTPLUG_CPU
Signature · 1 line
Jean Delvare
Quoted 10 lines · Jean Delvare
Hi Silas,
Quoted 5 lines · Silas Boyd-Wickizer
via_cputemp_initindrivers/hwmon/via-cputemp.cloops withfor_each_online_cpu, addingplatform_devices, then callsregister_hotcpu_notifier. If a CPU is offlined between the loop andregister_hotcpu_notifier, then later onlined,via_cputemp_device_addwill attempt to platform devices with the same ID.Missing word in this last sentence.
Fixed.
Quoted 9 lines
Quoted 6 lines
This fix surrounds
for_each_online_cpuandregister_hotcpu_notifierwith get_online_cpus+put_online_cpus.Build tested.
Thanks for reporting and for the fix. Two questions:
Quoted 8 lines
What about
via_cputemp_exit()? While less obvious, I suspect it is racy too. The notifier is unregistered first. If a CPU gets offline before the devices are removed, we will have a device pointing to a non-existent CPU for a short time. I think we should play it safe and use get/put_online_cpus() there too, as I seem to understand it guarantees CPUs can't go offline when we wouldn't handle that event properly. Alternatively, unregistering the platform driver first might close the race.
I added get/put_online_cpus().
Quoted 3 lines
Secondly,
drivers/hwmon/coretemp.cis very similar tovia-cputemp.c, so I think it needs the exact same fix(es).
Indeed -- I'll submit a fix for this.
Silas
Quoted 38 lines
Quoted 34 lines
Signed-off-by Silas Boyd-Wickizer <[email protected]>--- drivers/hwmon/via-cputemp.c | 3 +++ 1 file changed, 3 insertions(+)drivers/hwmon/via-cputemp.c +3
-0@@ -328,6 +328,7 @@ static int __init via_cputemp_init(void)if (err) goto exit; +get_online_cpus ();for_each_online_cpu (i) {struct cpuinfo_x86 *c = &cpu_data (i);@@ -347,12 +348,14 @@ static int __init via_cputemp_init(void)#ifndef CONFIG_HOTPLUG_CPU if (list_empty (&pdev_list)) { +put_online_cpus (); err = -ENODEV ; goto exit_driver_unreg; }#endif register_hotcpu_notifier (&via_cputemp_cpu_notifier); +put_online_cpus ();return 0 ;#ifndef CONFIG_HOTPLUG_CPU Signature · 1 line
Jean Delvare