fix BUG: using smp_processor_id() in preemptible code (resend)

fix BUG: using smp_processor_id() in preemptible code (resend)

[Sergey Senozhatsky]

Hello,

[  100.666169] lockdep: fixing up alternatives.
[  100.666183] SMP alternatives: switching to UP code
[  106.942505] BUG: using smp_processor_id() in preemptible [00000000] code: bash/5165
[  106.942541] caller is thermal_throttle_add_dev+0x20/0xa4
[  106.942548] Pid: 5165, comm: bash Not tainted 2.6.36-rc1-dbg-git1-00187-g358afc9-dirty #132
[  106.942553] Call Trace:
[  106.942565]  [<ffffffff811f6c01>] debug_smp_processor_id+0xc9/0xe4
[  106.942571]  [<ffffffff8136d377>] thermal_throttle_add_dev+0x20/0xa4
[  106.942578]  [<ffffffff8136d44d>] thermal_throttle_cpu_callback+0x52/0xb5
[  106.942587]  [<ffffffff810573f8>] notifier_call_chain+0x32/0x5e
[  106.942595]  [<ffffffff8103c840>] ? cpu_maps_update_begin+0x12/0x14
[  106.942602]  [<ffffffff81057443>] __raw_notifier_call_chain+0x9/0xb
[  106.942609]  [<ffffffff8103c703>] __cpu_notify+0x1b/0x2d
[  106.942616]  [<ffffffff8136f57d>] _cpu_up+0x6b/0xe9
[  106.942622]  [<ffffffff8136f645>] cpu_up+0x4a/0x57
[  106.942629]  [<ffffffff81363f77>] store_online+0x41/0x6e
[  106.942637]  [<ffffffff81291b4f>] sysdev_store+0x1b/0x1d
[  106.942644]  [<ffffffff81132186>] sysfs_write_file+0x103/0x13f
[  106.942653]  [<ffffffff810db026>] vfs_write+0xb0/0x14f
[  106.942659]  [<ffffffff810db2c2>] sys_write+0x45/0x6c
[  106.942668]  [<ffffffff81002002>] system_call_fastpath+0x16/0x1b


I have several solutions. Please kindly review.
The first one is:

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..1269661 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -205,7 +205,9 @@ static int therm_throt_process(bool new_event, int event, int level)
 static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
 {
 	int err;
-	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+	int cpu = get_cpu();
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	put_cpu();
 
 	err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
 	if (err)



Should we call smp_processor_id/get_cpu in thermal_throttle_add_dev?
We know the cpu when calling thermal_throttle_add_dev (in thermal_throttle_cpu_callback and
thermal_throttle_init_device) so we can pass the cpu number when calling it.

My second solution is:

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..bcd8499 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -202,11 +202,12 @@ static int therm_throt_process(bool new_event, int event, int level)
 
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
-static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
+static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
+				unsigned int cpu)
 {
 	int err;
-	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
-
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	
 	err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
 	if (err)
 		return err;
@@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
 		mutex_lock(&therm_cpu_lock);
-		err = thermal_throttle_add_dev(sys_dev);
+		err = thermal_throttle_add_dev(sys_dev, cpu);
 		mutex_unlock(&therm_cpu_lock);
 		WARN_ON(err);
 		break;
@@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
 #endif
 	/* connect live CPUs to sysfs */
 	for_each_online_cpu(cpu) {
-		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
+		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
 		WARN_ON(err);
 	}
 #ifdef CONFIG_HOTPLUG_CPU

Re: fix BUG: using smp_processor_id() in preemptible code (resend)

[Maciej Rutecki]

On środa, 18 sierpnia 2010 o 11:11:58 Sergey Senozhatsky wrote:
> fix BUG: using smp_processor_id() in preemptible code (resend)

I got similar bug (during suspend to disk) on 2.6.36-rc1+ patch from:
https://bugzilla.kernel.org/show_bug.cgi?id=16615

[   89.141370] Enabling non-boot CPUs ...
[   89.142033] BUG: using smp_processor_id() in preemptible [00000000] code: 
pm-hibernate/2584
[   89.142147] caller is thermal_throttle_add_dev+0x10/0x7c
[   89.142200] Pid: 2584, comm: pm-hibernate Not tainted 2.6.36-rc1 #1
[   89.142248] Call Trace:
[   89.142274]  [<c03706ad>] ? printk+0xf/0x12
[   89.142312]  [<c02181d1>] debug_smp_processor_id+0xa5/0xbc
[   89.142357]  [<c036d13d>] thermal_throttle_add_dev+0x10/0x7c
[   89.142403]  [<c036d1f2>] thermal_throttle_cpu_callback+0x49/0x9c
[   89.142454]  [<c0141842>] notifier_call_chain+0x26/0x50
[   89.142496]  [<c01418c3>] __raw_notifier_call_chain+0xe/0x10
[   89.142544]  [<c012be4a>] __cpu_notify+0x15/0x29
[   89.142581]  [<c036ead6>] _cpu_up+0x59/0xdc
[   89.142617]  [<c0364953>] enable_nonboot_cpus+0x3d/0x94
[   89.142660]  [<c0151f94>] hibernation_snapshot+0x11d/0x1b9
[   89.142704]  [<c01520d5>] hibernate+0xa5/0x13f
[   89.142740]  [<c0151035>] ? state_store+0x0/0xa7
[   89.142778]  [<c0151089>] state_store+0x54/0xa7
[   89.142815]  [<c0151035>] ? state_store+0x0/0xa7
[   89.142854]  [<c020ee89>] kobj_attr_store+0x16/0x22
[   89.142894]  [<c01c9a9a>] sysfs_write_file+0xbc/0xe7
[   89.142936]  [<c018e5d3>] vfs_write+0x8a/0xfd
[   89.142972]  [<c01c99de>] ? sysfs_write_file+0x0/0xe7
[   89.143018]  [<c018e6dd>] sys_write+0x3b/0x60
[   89.143055]  [<c01026ec>] sysenter_do_call+0x12/0x22
[   89.143112] SMP alternatives: switching to SMP code

PS. Sorry for overlook previosus mail, already I created bug entry (because 
it's regression):
https://bugzilla.kernel.org/show_bug.cgi?id=16629

Regards
-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: fix BUG: using smp_processor_id() in preemptible code (resend)

[Roedel, Joerg]

On Wed, Aug 18, 2010 at 05:11:58AM -0400, Sergey Senozhatsky wrote:
> Should we call smp_processor_id/get_cpu in thermal_throttle_add_dev?
> We know the cpu when calling thermal_throttle_add_dev (in thermal_throttle_cpu_callback and
> thermal_throttle_init_device) so we can pass the cpu number when calling it.
> 
> My second solution is:
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index c2a8b26..bcd8499 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -202,11 +202,12 @@ static int therm_throt_process(bool new_event, int event, int level)
>  
>  #ifdef CONFIG_SYSFS
>  /* Add/Remove thermal_throttle interface for CPU device: */
> -static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
> +static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> +				unsigned int cpu)
>  {
>  	int err;
> -	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> -
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	
>  	err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
>  	if (err)
>  		return err;
> @@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
>  	case CPU_UP_PREPARE:
>  	case CPU_UP_PREPARE_FROZEN:
>  		mutex_lock(&therm_cpu_lock);
> -		err = thermal_throttle_add_dev(sys_dev);
> +		err = thermal_throttle_add_dev(sys_dev, cpu);
>  		mutex_unlock(&therm_cpu_lock);
>  		WARN_ON(err);
>  		break;
> @@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
>  #endif
>  	/* connect live CPUs to sysfs */
>  	for_each_online_cpu(cpu) {
> -		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
> +		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
>  		WARN_ON(err);
>  	}
>  #ifdef CONFIG_HOTPLUG_CPU

The notifier is preemtible so it can run on another CPU. So using the
supplied cpu arguemnt in the path looks like the right solution to me.

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: fix BUG: using smp_processor_id() in preemptible code (resend)

[H. Peter Anvin]

On 08/19/2010 10:03 AM, Roedel, Joerg wrote:
> 
> The notifier is preemtible so it can run on another CPU. So using the
> supplied cpu arguemnt in the path looks like the right solution to me.
> 

Sounds good -- Sergey, can you send a patch under cleaned up cover
(meaning just the one patch, with a description of why the change is the
right one)?

	-hpa

[PATCH] fix BUG using smp_processor_id() in preemptible thermal_throttle_add_dev

[Sergey Senozhatsky]

Fix: using smp_processor_id() in preemptible thermal_throttle_add_dev.
We know the cpu number when calling thermal_throttle_add_dev, so we can
remove smp_processor_id call in thermal_throttle_add_dev by supplying 
the cpu number as argument.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..bcd8499 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -202,11 +202,12 @@ static int therm_throt_process(bool new_event, int event, int level)
 
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
-static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
+static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
+				unsigned int cpu)
 {
 	int err;
-	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
-
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	
 	err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
 	if (err)
 		return err;
@@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
 		mutex_lock(&therm_cpu_lock);
-		err = thermal_throttle_add_dev(sys_dev);
+		err = thermal_throttle_add_dev(sys_dev, cpu);
 		mutex_unlock(&therm_cpu_lock);
 		WARN_ON(err);
 		break;
@@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
 #endif
 	/* connect live CPUs to sysfs */
 	for_each_online_cpu(cpu) {
-		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
+		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
 		WARN_ON(err);
 	}
 #ifdef CONFIG_HOTPLUG_CPU

[tip:x86/urgent] x86, hwmon: Fix unsafe smp_processor_id() in thermal_throttle_add_dev

[tip-bot for Sergey Senozhatsky]

Commit-ID:  51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
Gitweb:     http://git.kernel.org/tip/51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
Author:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
AuthorDate: Fri, 20 Aug 2010 10:36:34 +0300
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Fri, 20 Aug 2010 19:56:00 -0700

x86, hwmon: Fix unsafe smp_processor_id() in thermal_throttle_add_dev

Fix BUG: using smp_processor_id() in preemptible thermal_throttle_add_dev.
We know the cpu number when calling thermal_throttle_add_dev, so we can
remove smp_processor_id call in thermal_throttle_add_dev by supplying
the cpu number as argument.

This should resolve kernel bugzilla 16615/16629.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
LKML-Reference: <20100820073634.GB5209@swordfish.minsk.epam.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Joerg Roedel <Joerg.Roedel@amd.com>
Cc: Maciej Rutecki <maciej.rutecki@gmail.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..d9368ee 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -202,10 +202,11 @@ static int therm_throt_process(bool new_event, int event, int level)
 
 #ifdef CONFIG_SYSFS
 /* Add/Remove thermal_throttle interface for CPU device: */
-static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
+static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
+				unsigned int cpu)
 {
 	int err;
-	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
 
 	err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
 	if (err)
@@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
 		mutex_lock(&therm_cpu_lock);
-		err = thermal_throttle_add_dev(sys_dev);
+		err = thermal_throttle_add_dev(sys_dev, cpu);
 		mutex_unlock(&therm_cpu_lock);
 		WARN_ON(err);
 		break;
@@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
 #endif
 	/* connect live CPUs to sysfs */
 	for_each_online_cpu(cpu) {
-		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
+		err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
 		WARN_ON(err);
 	}
 #ifdef CONFIG_HOTPLUG_CPU

Re: [tip:x86/urgent] x86, hwmon: Fix unsafe smp_processor_id() in thermal_throttle_add_dev

[Fenghua Yu]

On Fri, Aug 20, 2010 at 09:43:03PM -0700, tip-bot for Sergey Senozhatsky wrote:
> Commit-ID:  51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
> Gitweb:     http://git.kernel.org/tip/51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
> Author:     Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> AuthorDate: Fri, 20 Aug 2010 10:36:34 +0300
> Committer:  H. Peter Anvin <hpa@zytor.com>
> CommitDate: Fri, 20 Aug 2010 19:56:00 -0700
> 
> x86, hwmon: Fix unsafe smp_processor_id() in thermal_throttle_add_dev
> 
> Fix BUG: using smp_processor_id() in preemptible thermal_throttle_add_dev.
> We know the cpu number when calling thermal_throttle_add_dev, so we can
> remove smp_processor_id call in thermal_throttle_add_dev by supplying
> the cpu number as argument.
> 
> This should resolve kernel bugzilla 16615/16629.

The fix is right. Current thermal_throttle_add_dev checks local processor to
determine PTS and PLN features on another processor. Logically that is wrong.

Thanks.

-Fenghua