Re: [PATCH 3/3] Use get_online_cpus to avoid races involving for_each_online_cpu
On Fri, Aug 03, 2012 at 03:56:40PM +0200, Jean Delvare wrote: > Hi Silas, > > On Thu, 2 Aug 2012 17:07:08 -0700, Silas Boyd-Wickizer wrote: > > 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. > > Missing word in this last sentence. Fixed. > > > > > This fix surrounds for_each_online_cpu and register_hotcpu_notifier > > with 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. I added get/put_online_cpus(). > > Secondly, drivers/hwmon/coretemp.c is very similar to via-cputemp.c, so > I think it needs the exact same fix(es). Indeed -- I'll submit a fix for this. Silas > > > > > Signed-off-by: Silas Boyd-Wickizer > > --- > > drivers/hwmon/via-cputemp.c |3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c > > index 8689664..9ad07c3 100644 > > --- a/drivers/hwmon/via-cputemp.c > > +++ b/drivers/hwmon/via-cputemp.c > > @@ -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 = _data(i); > > > > @@ -347,12 +348,14 @@ static int __init via_cputemp_init(void) > > > > #ifndef CONFIG_HOTPLUG_CPU > > if (list_empty(_list)) { > > + put_online_cpus(); > > err = -ENODEV; > > goto exit_driver_unreg; > > } > > #endif > > > > register_hotcpu_notifier(_cputemp_cpu_notifier); > > + put_online_cpus(); > > return 0; > > > > #ifndef CONFIG_HOTPLUG_CPU > > > -- > Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Use get_online_cpus to avoid races involving for_each_online_cpu
Hi Silas, On Thu, 2 Aug 2012 17:07:08 -0700, Silas Boyd-Wickizer wrote: > 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. Missing word in this last sentence. > > This fix surrounds for_each_online_cpu and register_hotcpu_notifier > with 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). > > Signed-off-by: Silas Boyd-Wickizer > --- > drivers/hwmon/via-cputemp.c |3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c > index 8689664..9ad07c3 100644 > --- a/drivers/hwmon/via-cputemp.c > +++ b/drivers/hwmon/via-cputemp.c > @@ -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 = _data(i); > > @@ -347,12 +348,14 @@ static int __init via_cputemp_init(void) > > #ifndef CONFIG_HOTPLUG_CPU > if (list_empty(_list)) { > + put_online_cpus(); > err = -ENODEV; > goto exit_driver_unreg; > } > #endif > > register_hotcpu_notifier(_cputemp_cpu_notifier); > + put_online_cpus(); > return 0; > > #ifndef CONFIG_HOTPLUG_CPU -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Use get_online_cpus to avoid races involving for_each_online_cpu
Hi Silas, thanks a lot for the finding and addressing the issue. On Thu, Aug 02, 2012 at 05:07:08PM -0700, Silas Boyd-Wickizer wrote: > Signed-off-by: Silas Boyd-Wickizer Acked-by: Harald Welte -- - Harald Weltehttp://laforge.gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Use get_online_cpus to avoid races involving for_each_online_cpu
Hi Silas, thanks a lot for the finding and addressing the issue. On Thu, Aug 02, 2012 at 05:07:08PM -0700, Silas Boyd-Wickizer wrote: Signed-off-by: Silas Boyd-Wickizer s...@mit.edu Acked-by: Harald Welte lafo...@gnumonks.org -- - Harald Welte lafo...@gnumonks.org http://laforge.gnumonks.org/ Privacy in residential applications is a desirable marketing option. (ETSI EN 300 175-7 Ch. A6) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Use get_online_cpus to avoid races involving for_each_online_cpu
Hi Silas, On Thu, 2 Aug 2012 17:07:08 -0700, Silas Boyd-Wickizer wrote: 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. Missing word in this last sentence. This fix surrounds for_each_online_cpu and register_hotcpu_notifier with 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). Signed-off-by: Silas Boyd-Wickizer s...@mit.edu --- drivers/hwmon/via-cputemp.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c index 8689664..9ad07c3 100644 --- a/drivers/hwmon/via-cputemp.c +++ b/drivers/hwmon/via-cputemp.c @@ -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 -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Use get_online_cpus to avoid races involving for_each_online_cpu
On Fri, Aug 03, 2012 at 03:56:40PM +0200, Jean Delvare wrote: Hi Silas, On Thu, 2 Aug 2012 17:07:08 -0700, Silas Boyd-Wickizer wrote: 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. Missing word in this last sentence. Fixed. This fix surrounds for_each_online_cpu and register_hotcpu_notifier with 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. I added get/put_online_cpus(). Secondly, drivers/hwmon/coretemp.c is very similar to via-cputemp.c, so I think it needs the exact same fix(es). Indeed -- I'll submit a fix for this. Silas Signed-off-by: Silas Boyd-Wickizer s...@mit.edu --- drivers/hwmon/via-cputemp.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c index 8689664..9ad07c3 100644 --- a/drivers/hwmon/via-cputemp.c +++ b/drivers/hwmon/via-cputemp.c @@ -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 -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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. Signed-off-by: Silas Boyd-Wickizer --- drivers/hwmon/via-cputemp.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c index 8689664..9ad07c3 100644 --- a/drivers/hwmon/via-cputemp.c +++ b/drivers/hwmon/via-cputemp.c @@ -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 = _data(i); @@ -347,12 +348,14 @@ static int __init via_cputemp_init(void) #ifndef CONFIG_HOTPLUG_CPU if (list_empty(_list)) { + put_online_cpus(); err = -ENODEV; goto exit_driver_unreg; } #endif register_hotcpu_notifier(_cputemp_cpu_notifier); + put_online_cpus(); return 0; #ifndef CONFIG_HOTPLUG_CPU -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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. Signed-off-by: Silas Boyd-Wickizer s...@mit.edu --- drivers/hwmon/via-cputemp.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c index 8689664..9ad07c3 100644 --- a/drivers/hwmon/via-cputemp.c +++ b/drivers/hwmon/via-cputemp.c @@ -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 -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/