[PATCH AUTOSEL 4.9 3/4] powercap: intel_rapl: fix UBSAN shift-out-of-bounds issue

[PATCH AUTOSEL 4.9 3/4] powercap: intel_rapl: fix UBSAN shift-out-of-bounds issue

[Sasha Levin]

From: Chao Qin <chao.qin@intel.com>

[ Upstream commit 2d93540014387d1c73b9ccc4d7895320df66d01b ]

When value < time_unit, the parameter of ilog2() will be zero and
the return value is -1. u64(-1) is too large for shift exponent
and then will trigger shift-out-of-bounds:

shift exponent 18446744073709551615 is too large for 32-bit type 'int'
Call Trace:
 rapl_compute_time_window_core
 rapl_write_data_raw
 set_time_window
 store_constraint_time_window_us

Signed-off-by: Chao Qin <chao.qin@intel.com>
Acked-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/powercap/intel_rapl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index 8809c1a20bed..5f31606e1982 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -1080,6 +1080,9 @@ static u64 rapl_compute_time_window_core(struct rapl_package *rp, u64 value,
 		y = value & 0x1f;
 		value = (1 << y) * (4 + f) * rp->time_unit / 4;
 	} else {
+		if (value < rp->time_unit)
+			return 0;
+
 		do_div(value, rp->time_unit);
 		y = ilog2(value);
 		f = div64_u64(4 * (value - (1 << y)), 1 << y);
-- 
2.35.1

[PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash

[Sasha Levin]

From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

[ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]

When CPU 0 is offline and intel_powerclamp is used to inject
idle, it generates kernel BUG:

BUG: using smp_processor_id() in preemptible [00000000] code: bash/15687
caller is debug_smp_processor_id+0x17/0x20
CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
Call Trace:
<TASK>
dump_stack_lvl+0x49/0x63
dump_stack+0x10/0x16
check_preemption_disabled+0xdd/0xe0
debug_smp_processor_id+0x17/0x20
powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
...
...

Here CPU 0 is the control CPU by default and changed to the current CPU,
if CPU 0 offlined. This check has to be performed under cpus_read_lock(),
hence the above warning.

Use get_cpu() instead of smp_processor_id() to avoid this BUG.

Suggested-by: Chen Yu <yu.c.chen@intel.com>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
[ rjw: Subject edits ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/thermal/intel_powerclamp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/intel_powerclamp.c b/drivers/thermal/intel_powerclamp.c
index afada655f861..492bb3ec6546 100644
--- a/drivers/thermal/intel_powerclamp.c
+++ b/drivers/thermal/intel_powerclamp.c
@@ -519,8 +519,10 @@ static int start_power_clamp(void)
 
 	/* prefer BSP */
 	control_cpu = 0;
-	if (!cpu_online(control_cpu))
-		control_cpu = smp_processor_id();
+	if (!cpu_online(control_cpu)) {
+		control_cpu = get_cpu();
+		put_cpu();
+	}
 
 	clamping = true;
 	schedule_delayed_work(&poll_pkg_cstate_work, 0);
-- 
2.35.1

Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

Hi!

> From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> [ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]
> 
> When CPU 0 is offline and intel_powerclamp is used to inject
> idle, it generates kernel BUG:
> 
> BUG: using smp_processor_id() in preemptible [00000000] code: bash/15687
> caller is debug_smp_processor_id+0x17/0x20
> CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
> Call Trace:
> <TASK>
> dump_stack_lvl+0x49/0x63
> dump_stack+0x10/0x16
> check_preemption_disabled+0xdd/0xe0
> debug_smp_processor_id+0x17/0x20
> powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
> ...
> ...
> 
> Here CPU 0 is the control CPU by default and changed to the current CPU,
> if CPU 0 offlined. This check has to be performed under cpus_read_lock(),
> hence the above warning.
> 
> Use get_cpu() instead of smp_processor_id() to avoid this BUG.

This has exactly the same problem as smp_processor_id(), you just
worked around the warning. If it is okay that control_cpu contains
stale value, could we have a comment explaining why?

Thanks,
								Pavel
								
> +++ b/drivers/thermal/intel_powerclamp.c
> @@ -519,8 +519,10 @@ static int start_power_clamp(void)
>  
>  	/* prefer BSP */
>  	control_cpu = 0;
> -	if (!cpu_online(control_cpu))
> -		control_cpu = smp_processor_id();
> +	if (!cpu_online(control_cpu)) {
> +		control_cpu = get_cpu();
> +		put_cpu();
> +	}
>  
>  	clamping = true;
>  	schedule_delayed_work(&poll_pkg_cstate_work, 0);
> -- 
> 2.35.1

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash

[Chen Yu]

Hi Pavel,
On 2022-10-11 at 13:36:46 +0200, Pavel Machek wrote:
> Hi!
> 
> > From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > 
> > [ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]
> > 
> > When CPU 0 is offline and intel_powerclamp is used to inject
> > idle, it generates kernel BUG:
> > 
> > BUG: using smp_processor_id() in preemptible [00000000] code: bash/15687
> > caller is debug_smp_processor_id+0x17/0x20
> > CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
> > Call Trace:
> > <TASK>
> > dump_stack_lvl+0x49/0x63
> > dump_stack+0x10/0x16
> > check_preemption_disabled+0xdd/0xe0
> > debug_smp_processor_id+0x17/0x20
> > powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
> > ...
> > ...
> > 
> > Here CPU 0 is the control CPU by default and changed to the current CPU,
> > if CPU 0 offlined. This check has to be performed under cpus_read_lock(),
> > hence the above warning.
> > 
> > Use get_cpu() instead of smp_processor_id() to avoid this BUG.
> 
> This has exactly the same problem as smp_processor_id(), you just
> worked around the warning. If it is okay that control_cpu contains
> stale value, could we have a comment explaining why?
>
May I know why does control_cpu have stale value? The control_cpu
is a random picked online CPU which will be used later to collect statistics.
As long as the control_cpu is online, it is valid IMO.

thanks,
Chenyu
> Thanks,
> 								Pavel
> 								
> > +++ b/drivers/thermal/intel_powerclamp.c
> > @@ -519,8 +519,10 @@ static int start_power_clamp(void)
> >  
> >  	/* prefer BSP */
> >  	control_cpu = 0;
> > -	if (!cpu_online(control_cpu))
> > -		control_cpu = smp_processor_id();
> > +	if (!cpu_online(control_cpu)) {
> > +		control_cpu = get_cpu();
> > +		put_cpu();
> > +	}
> >  
> >  	clamping = true;
> >  	schedule_delayed_work(&poll_pkg_cstate_work, 0);
> > -- 
> > 2.35.1
> 
> -- 
> People of Russia, stop Putin before his war on Ukraine escalates.

Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash

[srinivas pandruvada]

On Tue, 2022-10-11 at 21:22 +0800, Chen Yu wrote:
> Hi Pavel,
> On 2022-10-11 at 13:36:46 +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > 
> > > [ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]
> > > 
> > > When CPU 0 is offline and intel_powerclamp is used to inject
> > > idle, it generates kernel BUG:
> > > 
> > > BUG: using smp_processor_id() in preemptible [00000000] code:
> > > bash/15687
> > > caller is debug_smp_processor_id+0x17/0x20
> > > CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
> > > Call Trace:
> > > <TASK>
> > > dump_stack_lvl+0x49/0x63
> > > dump_stack+0x10/0x16
> > > check_preemption_disabled+0xdd/0xe0
> > > debug_smp_processor_id+0x17/0x20
> > > powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
> > > ...
> > > ...
> > > 
> > > Here CPU 0 is the control CPU by default and changed to the current
> > > CPU,
> > > if CPU 0 offlined. This check has to be performed under
> > > cpus_read_lock(),
> > > hence the above warning.
> > > 
> > > Use get_cpu() instead of smp_processor_id() to avoid this BUG.
> > 
> > This has exactly the same problem as smp_processor_id(), you just
> > worked around the warning. If it is okay that control_cpu contains
> > stale value, could we have a comment explaining why?
> > 
> May I know why does control_cpu have stale value? The control_cpu
> is a random picked online CPU which will be used later to collect
> statistics.
> As long as the control_cpu is online, it is valid IMO.
> 

I am also interested to know why this can be stale. The get_cpu() call
disables preemption. 

#define get_cpu()		({ preempt_disable();
__smp_processor_id(); })


Even if you change it to call debug_smp_processor_id() instead of
__smp_processor_id(), it will still not print warning as
preempt_count() will return 1.

If after the preemption is enabled if the CPU is offlined, there are
hotplug callbacks to handle.


Thanks,
Srinivas
 

> thanks,
> Chenyu
> > Thanks,
> >                                                                 Pavel
> >                                                                 
> > > +++ b/drivers/thermal/intel_powerclamp.c
> > > @@ -519,8 +519,10 @@ static int start_power_clamp(void)
> > >  
> > >         /* prefer BSP */
> > >         control_cpu = 0;
> > > -       if (!cpu_online(control_cpu))
> > > -               control_cpu = smp_processor_id();
> > > +       if (!cpu_online(control_cpu)) {
> > > +               control_cpu = get_cpu();
> > > +               put_cpu();
> > > +       }
> > >  
> > >         clamping = true;
> > >         schedule_delayed_work(&poll_pkg_cstate_work, 0);
> > > -- 
> > > 2.35.1
> > 
> > -- 
> > People of Russia, stop Putin before his war on Ukraine escalates.
> 
> 

Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash

[Rafael J. Wysocki]

On Tue, Oct 11, 2022 at 3:23 PM Chen Yu <yu.c.chen@intel.com> wrote:
>
> Hi Pavel,
> On 2022-10-11 at 13:36:46 +0200, Pavel Machek wrote:
> > Hi!
> >
> > > From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > >
> > > [ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]
> > >
> > > When CPU 0 is offline and intel_powerclamp is used to inject
> > > idle, it generates kernel BUG:
> > >
> > > BUG: using smp_processor_id() in preemptible [00000000] code: bash/15687
> > > caller is debug_smp_processor_id+0x17/0x20
> > > CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
> > > Call Trace:
> > > <TASK>
> > > dump_stack_lvl+0x49/0x63
> > > dump_stack+0x10/0x16
> > > check_preemption_disabled+0xdd/0xe0
> > > debug_smp_processor_id+0x17/0x20
> > > powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
> > > ...
> > > ...
> > >
> > > Here CPU 0 is the control CPU by default and changed to the current CPU,
> > > if CPU 0 offlined. This check has to be performed under cpus_read_lock(),
> > > hence the above warning.
> > >
> > > Use get_cpu() instead of smp_processor_id() to avoid this BUG.
> >
> > This has exactly the same problem as smp_processor_id(), you just
> > worked around the warning. If it is okay that control_cpu contains
> > stale value, could we have a comment explaining why?
> >
> May I know why does control_cpu have stale value? The control_cpu
> is a random picked online CPU which will be used later to collect statistics.
> As long as the control_cpu is online, it is valid IMO.

So this is confusing, because the code makes the impression that
getting the number of the CPU running the code matters in some way,
which isn't the case.

Something like cpumask_first(cpu_online_mask) should work as well if
I'm not mistaken and it would be less confusing to use this instead
IMO.

Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash

[srinivas pandruvada]

On Wed, 2022-10-12 at 18:58 +0200, Rafael J. Wysocki wrote:
> On Tue, Oct 11, 2022 at 3:23 PM Chen Yu <yu.c.chen@intel.com> wrote:
> > 
> > Hi Pavel,
> > On 2022-10-11 at 13:36:46 +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > > 
> > > > [ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]
> > > > 
> > > > When CPU 0 is offline and intel_powerclamp is used to inject
> > > > idle, it generates kernel BUG:
> > > > 
> > > > BUG: using smp_processor_id() in preemptible [00000000] code:
> > > > bash/15687
> > > > caller is debug_smp_processor_id+0x17/0x20
> > > > CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
> > > > Call Trace:
> > > > <TASK>
> > > > dump_stack_lvl+0x49/0x63
> > > > dump_stack+0x10/0x16
> > > > check_preemption_disabled+0xdd/0xe0
> > > > debug_smp_processor_id+0x17/0x20
> > > > powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
> > > > ...
> > > > ...
> > > > 
> > > > Here CPU 0 is the control CPU by default and changed to the
> > > > current CPU,
> > > > if CPU 0 offlined. This check has to be performed under
> > > > cpus_read_lock(),
> > > > hence the above warning.
> > > > 
> > > > Use get_cpu() instead of smp_processor_id() to avoid this BUG.
> > > 
> > > This has exactly the same problem as smp_processor_id(), you just
> > > worked around the warning. If it is okay that control_cpu
> > > contains
> > > stale value, could we have a comment explaining why?
> > > 
> > May I know why does control_cpu have stale value? The control_cpu
> > is a random picked online CPU which will be used later to collect
> > statistics.
> > As long as the control_cpu is online, it is valid IMO.
> 
> So this is confusing, because the code makes the impression that
> getting the number of the CPU running the code matters in some way,
> which isn't the case.
> 
> Something like cpumask_first(cpu_online_mask) should work as well if
> I'm not mistaken and it would be less confusing to use this instead
> IMO.
That should work as we are under hotplug lock anyway here.

Thanks,
Srinivas

Re: [PATCH AUTOSEL 4.9 4/4] thermal: intel_powerclamp: Use get_cpu() instead of smp_processor_id() to avoid crash

[Rafael J. Wysocki]

On Thu, Oct 13, 2022 at 5:06 AM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Wed, 2022-10-12 at 18:58 +0200, Rafael J. Wysocki wrote:
> > On Tue, Oct 11, 2022 at 3:23 PM Chen Yu <yu.c.chen@intel.com> wrote:
> > >
> > > Hi Pavel,
> > > On 2022-10-11 at 13:36:46 +0200, Pavel Machek wrote:
> > > > Hi!
> > > >
> > > > > From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > > >
> > > > > [ Upstream commit 68b99e94a4a2db6ba9b31fe0485e057b9354a640 ]
> > > > >
> > > > > When CPU 0 is offline and intel_powerclamp is used to inject
> > > > > idle, it generates kernel BUG:
> > > > >
> > > > > BUG: using smp_processor_id() in preemptible [00000000] code:
> > > > > bash/15687
> > > > > caller is debug_smp_processor_id+0x17/0x20
> > > > > CPU: 4 PID: 15687 Comm: bash Not tainted 5.19.0-rc7+ #57
> > > > > Call Trace:
> > > > > <TASK>
> > > > > dump_stack_lvl+0x49/0x63
> > > > > dump_stack+0x10/0x16
> > > > > check_preemption_disabled+0xdd/0xe0
> > > > > debug_smp_processor_id+0x17/0x20
> > > > > powerclamp_set_cur_state+0x7f/0xf9 [intel_powerclamp]
> > > > > ...
> > > > > ...
> > > > >
> > > > > Here CPU 0 is the control CPU by default and changed to the
> > > > > current CPU,
> > > > > if CPU 0 offlined. This check has to be performed under
> > > > > cpus_read_lock(),
> > > > > hence the above warning.
> > > > >
> > > > > Use get_cpu() instead of smp_processor_id() to avoid this BUG.
> > > >
> > > > This has exactly the same problem as smp_processor_id(), you just
> > > > worked around the warning. If it is okay that control_cpu
> > > > contains
> > > > stale value, could we have a comment explaining why?
> > > >
> > > May I know why does control_cpu have stale value? The control_cpu
> > > is a random picked online CPU which will be used later to collect
> > > statistics.
> > > As long as the control_cpu is online, it is valid IMO.
> >
> > So this is confusing, because the code makes the impression that
> > getting the number of the CPU running the code matters in some way,
> > which isn't the case.
> >
> > Something like cpumask_first(cpu_online_mask) should work as well if
> > I'm not mistaken and it would be less confusing to use this instead
> > IMO.
> That should work as we are under hotplug lock anyway here.

Well, that's my point.

I guess I'll send a patch with this change.