eric
2012-Mar-07 14:07 UTC
[PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array
Calling XENPF_set_processor_pminfo with XEN_PM_CX could cause states
array in "struct acpi_processor_power" to exceed its limit.
The array used to be reset (by function cpuidle_init_cpu()) for each
hypercall. The patch puts it back that way and adds an assertion to make
it clear in case that happens again.
Signed-off-by: Eric Chanudet <eric.chanudet@eu.citrix.com>
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -622,19 +622,19 @@ static int cpuidle_init_cpu(int cpu)
for ( i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++ )
acpi_power->states[i].idx = i;
-
- acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1;
- acpi_power->states[ACPI_STATE_C1].entry_method =
ACPI_CSTATE_EM_HALT;
-
- acpi_power->states[ACPI_STATE_C0].valid = 1;
- acpi_power->states[ACPI_STATE_C1].valid = 1;
-
- acpi_power->count = 2;
- acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1];
- acpi_power->cpu = cpu;
processor_powers[cpu] = acpi_power;
}
+ acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1;
+ acpi_power->states[ACPI_STATE_C1].entry_method = ACPI_CSTATE_EM_HALT;
+
+ acpi_power->states[ACPI_STATE_C0].valid = 1;
+ acpi_power->states[ACPI_STATE_C1].valid = 1;
+
+ acpi_power->count = 2;
+ acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1];
+ acpi_power->cpu = cpu;
+
if ( cpu == 0 )
{
if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
@@ -870,11 +870,10 @@ static void set_cx(
if ( xen_cx->type == ACPI_STATE_C1 )
cx = &acpi_power->states[1];
- else
- cx = &acpi_power->states[acpi_power->count];
-
- if ( !cx->valid )
- acpi_power->count++;
+ else {
+ ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER);
+ cx = &acpi_power->states[acpi_power->count++];
+ }
cx->valid = 1;
cx->type = xen_cx->type;
--
Eric Chanudet
Jan Beulich
2012-Mar-07 15:07 UTC
Re: [PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array
>>> On 07.03.12 at 15:07, eric <eric.chanudet@citrix.com> wrote: > Calling XENPF_set_processor_pminfo with XEN_PM_CX could cause states > array in "struct acpi_processor_power" to exceed its limit. > > The array used to be reset (by function cpuidle_init_cpu()) for each > hypercall. The patch puts it back that way and adds an assertion to make > it clear in case that happens again. > > Signed-off-by: Eric Chanudet <eric.chanudet@eu.citrix.com> > > diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c > --- a/xen/arch/x86/acpi/cpu_idle.c > +++ b/xen/arch/x86/acpi/cpu_idle.c > @@ -622,19 +622,19 @@ static int cpuidle_init_cpu(int cpu) > > for ( i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++ ) > acpi_power->states[i].idx = i; > - > - acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1; > - acpi_power->states[ACPI_STATE_C1].entry_method = ACPI_CSTATE_EM_HALT; > - > - acpi_power->states[ACPI_STATE_C0].valid = 1; > - acpi_power->states[ACPI_STATE_C1].valid = 1; > - > - acpi_power->count = 2; > - acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1]; > - acpi_power->cpu = cpu; > processor_powers[cpu] = acpi_power; > } > > + acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1; > + acpi_power->states[ACPI_STATE_C1].entry_method = ACPI_CSTATE_EM_HALT; > + > + acpi_power->states[ACPI_STATE_C0].valid = 1; > + acpi_power->states[ACPI_STATE_C1].valid = 1; > + > + acpi_power->count = 2; > + acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1]; > + acpi_power->cpu = cpu;Is there any reason to move all of these assignments, rather than just the assignment of ->count and maybe ->states[ACPI_STATE_C1]? Everything else should not get modified anywhere else.> + > if ( cpu == 0 ) > { > if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) ) > @@ -870,11 +870,10 @@ static void set_cx( > > if ( xen_cx->type == ACPI_STATE_C1 ) > cx = &acpi_power->states[1]; > - else > - cx = &acpi_power->states[acpi_power->count]; > - > - if ( !cx->valid ) > - acpi_power->count++; > + else {Please put the brace on a new line.> + ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER); > + cx = &acpi_power->states[acpi_power->count++]; > + }To be on the safe side, you should probably also check for ACPI_STATE_C0 (and ignore its data), otherwise (other than before your patch) ->count would get incremented for this too. So I''d suggest converting the if() here to a switch(). Jan> > cx->valid = 1; > cx->type = xen_cx->type; > > -- > Eric Chanudet > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Eric Chanudet
2012-Mar-07 17:46 UTC
Re: [PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array
On Wed, Mar 07, 2012 at 10:07:55AM -0500, Jan Beulich wrote:> Is there any reason to move all of these assignments, rather than > just the assignment of ->count and maybe ->states[ACPI_STATE_C1]? > Everything else should not get modified anywhere else.I see no reason indeed, this is how it was before everything went up in the if() statement.> To be on the safe side, you should probably also check for > ACPI_STATE_C0 (and ignore its data), otherwise (other than before > your patch) ->count would get incremented for this too. So I''d > suggest converting the if() here to a switch().Done. Here is the refreshed patch. diff -r 8964c223836c xen/arch/x86/acpi/cpu_idle.c --- a/xen/arch/x86/acpi/cpu_idle.c Tue Mar 06 15:51:33 2012 +0100 +++ b/xen/arch/x86/acpi/cpu_idle.c Wed Mar 07 17:39:34 2012 +0000 @@ -645,12 +645,12 @@ acpi_power->states[ACPI_STATE_C0].valid = 1; acpi_power->states[ACPI_STATE_C1].valid = 1; + acpi_power->cpu = cpu; + processor_powers[cpu] = acpi_power; + } acpi_power->count = 2; acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1]; - acpi_power->cpu = cpu; - processor_powers[cpu] = acpi_power; - } if ( cpu == 0 ) { @@ -885,16 +885,20 @@ if ( check_cx(acpi_power, xen_cx) != 0 ) return; - if ( xen_cx->type == ACPI_STATE_C1 ) + switch (xen_cx->type) + { + case ACPI_STATE_C0: + return; + case ACPI_STATE_C1: cx = &acpi_power->states[1]; - else - cx = &acpi_power->states[acpi_power->count]; + break; + default: + ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER); + cx = &acpi_power->states[acpi_power->count++]; + cx->valid = 1; + cx->type = xen_cx->type; + } - if ( !cx->valid ) - acpi_power->count++; - - cx->valid = 1; - cx->type = xen_cx->type; cx->address = xen_cx->reg.address; switch ( xen_cx->reg.space_id ) -- Eric Chanudet
Zhang, Yang Z
2012-Mar-08 00:39 UTC
Re: [PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array
Sorry, can you give more descriptions about this issue? I doesn''t see any problem. best regards yang> -----Original Message----- > From: xen-devel-bounces@lists.xen.org > [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of eric > Sent: Wednesday, March 07, 2012 10:07 PM > To: xen-devel@lists.xen.org > Cc: julian.pidancet@eu.citrix.com > Subject: [Xen-devel] [PATCH] XENPF_set_processor_pminfo XEN_PM_CX > overflows states array > > Calling XENPF_set_processor_pminfo with XEN_PM_CX could cause states array > in "struct acpi_processor_power" to exceed its limit. > > The array used to be reset (by function cpuidle_init_cpu()) for each hypercall. > The patch puts it back that way and adds an assertion to make it clear in case that > happens again. > > Signed-off-by: Eric Chanudet <eric.chanudet@eu.citrix.com> > > diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c > --- a/xen/arch/x86/acpi/cpu_idle.c > +++ b/xen/arch/x86/acpi/cpu_idle.c > @@ -622,19 +622,19 @@ static int cpuidle_init_cpu(int cpu) > > for ( i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++ ) > acpi_power->states[i].idx = i; > - > - acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1; > - acpi_power->states[ACPI_STATE_C1].entry_method > ACPI_CSTATE_EM_HALT; > - > - acpi_power->states[ACPI_STATE_C0].valid = 1; > - acpi_power->states[ACPI_STATE_C1].valid = 1; > - > - acpi_power->count = 2; > - acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1]; > - acpi_power->cpu = cpu; > processor_powers[cpu] = acpi_power; > } > > + acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1; > + acpi_power->states[ACPI_STATE_C1].entry_method > + ACPI_CSTATE_EM_HALT; > + > + acpi_power->states[ACPI_STATE_C0].valid = 1; > + acpi_power->states[ACPI_STATE_C1].valid = 1; > + > + acpi_power->count = 2; > + acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1]; > + acpi_power->cpu = cpu; > + > if ( cpu == 0 ) > { > if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) ) @@ -870,11 > +870,10 @@ static void set_cx( > > if ( xen_cx->type == ACPI_STATE_C1 ) > cx = &acpi_power->states[1]; > - else > - cx = &acpi_power->states[acpi_power->count]; > - > - if ( !cx->valid ) > - acpi_power->count++; > + else { > + ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER); > + cx = &acpi_power->states[acpi_power->count++]; > + } > > cx->valid = 1; > cx->type = xen_cx->type; > > -- > Eric Chanudet > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Jan Beulich
2012-Mar-08 08:34 UTC
Re: [PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array
>>> On 07.03.12 at 18:46, Eric Chanudet <eric.chanudet@eu.citrix.com> wrote: > --- a/xen/arch/x86/acpi/cpu_idle.c Tue Mar 06 15:51:33 2012 +0100 > +++ b/xen/arch/x86/acpi/cpu_idle.c Wed Mar 07 17:39:34 2012 +0000 > @@ -645,12 +645,12 @@ > > acpi_power->states[ACPI_STATE_C0].valid = 1; > acpi_power->states[ACPI_STATE_C1].valid = 1; > + acpi_power->cpu = cpu; > + processor_powers[cpu] = acpi_power; > + } > > acpi_power->count = 2; > acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1];Please fix the indentation here. Further, I''d really like to see the two C1-related lines (setting .type and .entry_method) moved out of the if() as well - if we''re getting fresh data from Dom0, we shouldn''t make assumptions on the validity of the old data.> - acpi_power->cpu = cpu; > - processor_powers[cpu] = acpi_power; > - } > > if ( cpu == 0 ) > {Further, as you''re adjusting this anyway, I think this whole if() should move into the earlier if()''s body.> @@ -885,16 +885,20 @@ > if ( check_cx(acpi_power, xen_cx) != 0 ) > return; > > - if ( xen_cx->type == ACPI_STATE_C1 ) > + switch (xen_cx->type) > + { > + case ACPI_STATE_C0: > + return; > + case ACPI_STATE_C1: > cx = &acpi_power->states[1]; > - else > - cx = &acpi_power->states[acpi_power->count]; > + break; > + default: > + ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER);After some more thinking about this, I don''t see ASSERT() as a proper action here. Rather than crashing the debug hypervisor and silently corrupting data on a non-debug one, print a warning here and bail out.> + cx = &acpi_power->states[acpi_power->count++]; > + cx->valid = 1; > + cx->type = xen_cx->type; > + } > > - if ( !cx->valid ) > - acpi_power->count++; > - > - cx->valid = 1; > - cx->type = xen_cx->type; > cx->address = xen_cx->reg.address; > > switch ( xen_cx->reg.space_id )As the patch is sufficiently different from the previous version, I think you ought to re-submit with a full patch description and s-o-b line. Or maybe I should just take your patch as a basis and do the adjustments myself... Jan
Jan Beulich
2012-Mar-08 10:53 UTC
Re: [PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array
>>> On 08.03.12 at 01:39, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote: > Sorry, can you give more descriptions about this issue? I doesn''t see any > problem.This is pretty obvious once you know what to look for: Just consider Dom0 issues the respective hypercall twice for each CPU. cpuidle_init_cpu() in its unpatched form won''t reset acpi_power->count back to 2, yet set_cx() will continue to happily pick fresh array slots and increment the count further (unless the ->valid flag happens to be set in this uninitialized chunk of memory). Iiuc this should be easily reproducible unloading and reloading processor.ko on a XenoLinux kernel. Jan>> -----Original Message----- >> From: xen-devel-bounces@lists.xen.org >> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of eric >> Sent: Wednesday, March 07, 2012 10:07 PM >> To: xen-devel@lists.xen.org >> Cc: julian.pidancet@eu.citrix.com >> Subject: [Xen-devel] [PATCH] XENPF_set_processor_pminfo XEN_PM_CX >> overflows states array >> >> Calling XENPF_set_processor_pminfo with XEN_PM_CX could cause states array >> in "struct acpi_processor_power" to exceed its limit. >> >> The array used to be reset (by function cpuidle_init_cpu()) for each > hypercall. >> The patch puts it back that way and adds an assertion to make it clear in > case that >> happens again. >> >> Signed-off-by: Eric Chanudet <eric.chanudet@eu.citrix.com> >> >> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c >> --- a/xen/arch/x86/acpi/cpu_idle.c >> +++ b/xen/arch/x86/acpi/cpu_idle.c >> @@ -622,19 +622,19 @@ static int cpuidle_init_cpu(int cpu) >> >> for ( i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++ ) >> acpi_power->states[i].idx = i; >> - >> - acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1; >> - acpi_power->states[ACPI_STATE_C1].entry_method >> ACPI_CSTATE_EM_HALT; >> - >> - acpi_power->states[ACPI_STATE_C0].valid = 1; >> - acpi_power->states[ACPI_STATE_C1].valid = 1; >> - >> - acpi_power->count = 2; >> - acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1]; >> - acpi_power->cpu = cpu; >> processor_powers[cpu] = acpi_power; >> } >> >> + acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1; >> + acpi_power->states[ACPI_STATE_C1].entry_method >> + ACPI_CSTATE_EM_HALT; >> + >> + acpi_power->states[ACPI_STATE_C0].valid = 1; >> + acpi_power->states[ACPI_STATE_C1].valid = 1; >> + >> + acpi_power->count = 2; >> + acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1]; >> + acpi_power->cpu = cpu; >> + >> if ( cpu == 0 ) >> { >> if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) ) @@ -870,11 >> +870,10 @@ static void set_cx( >> >> if ( xen_cx->type == ACPI_STATE_C1 ) >> cx = &acpi_power->states[1]; >> - else >> - cx = &acpi_power->states[acpi_power->count]; >> - >> - if ( !cx->valid ) >> - acpi_power->count++; >> + else { >> + ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER); >> + cx = &acpi_power->states[acpi_power->count++]; >> + } >> >> cx->valid = 1; >> cx->type = xen_cx->type; >> >> -- >> Eric Chanudet >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Jan Beulich
2012-Mar-08 12:45 UTC
[PATCH] Re: XENPF_set_processor_pminfo XEN_PM_CX overflows states array
Calling XENPF_set_processor_pminfo with XEN_PM_CX could cause states
array in "struct acpi_processor_power" to exceed its limit.
The array used to be reset (by function cpuidle_init_cpu()) for each
hypercall. The patch puts it back that way and adds an assertion to
make it clear in case that happens again.
Signed-off-by: Eric Chanudet <eric.chanudet@eu.citrix.com>
- convert assertion to printk() & bail
- eliminate struct acpi_processor_cx''s valid member (not read anymore)
- further adjustments to one-time-only vs each-time operations in
cpuidle_init_cpu()
- don''t use ACPI_STATE_Cn as array index anymore
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- 2012-03-06.orig/xen/arch/x86/acpi/cpu_idle.c 2012-03-06 13:35:15.000000000
+0100
+++ 2012-03-06/xen/arch/x86/acpi/cpu_idle.c 2012-03-08 12:36:24.000000000 +0100
@@ -73,10 +73,8 @@ static void lapic_timer_nop(void) { }
static void (*lapic_timer_off)(void);
static void (*lapic_timer_on)(void);
-static uint64_t (*get_tick)(void);
-static uint64_t (*ticks_elapsed)(uint64_t t1, uint64_t t2);
-static uint64_t (*tick_to_ns)(uint64_t ticks);
-static uint64_t (*ns_to_tick)(uint64_t ticks);
+static uint64_t (*__read_mostly tick_to_ns)(uint64_t) = acpi_pm_tick_to_ns;
+static uint64_t (*__read_mostly ns_to_tick)(uint64_t) = ns_to_acpi_pm_tick;
static void (*pm_idle_save) (void) __read_mostly;
unsigned int max_cstate __read_mostly = ACPI_PROCESSOR_MAX_POWER - 1;
@@ -234,6 +232,10 @@ static uint64_t acpi_pm_ticks_elapsed(ui
return ((0xFFFFFFFF - t1) + t2 +1);
}
+static uint64_t (*__read_mostly get_tick)(void) = get_acpi_pm_tick;
+static uint64_t (*__read_mostly ticks_elapsed)(uint64_t, uint64_t)
+ = acpi_pm_ticks_elapsed;
+
#define MWAIT_ECX_INTERRUPT_BREAK (0x1)
/*
@@ -632,43 +634,31 @@ static int cpuidle_init_cpu(int cpu)
acpi_power = processor_powers[cpu];
if ( !acpi_power )
{
- int i;
+ unsigned int i;
+
+ if ( cpu == 0 && boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
+ {
+ get_tick = get_stime_tick;
+ ticks_elapsed = stime_ticks_elapsed;
+ tick_to_ns = stime_tick_to_ns;
+ ns_to_tick = ns_to_stime_tick;
+ }
+
acpi_power = xzalloc(struct acpi_processor_power);
if ( !acpi_power )
return -ENOMEM;
for ( i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++ )
acpi_power->states[i].idx = i;
-
- acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1;
- acpi_power->states[ACPI_STATE_C1].entry_method =
ACPI_CSTATE_EM_HALT;
-
- acpi_power->states[ACPI_STATE_C0].valid = 1;
- acpi_power->states[ACPI_STATE_C1].valid = 1;
-
- acpi_power->count = 2;
- acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1];
+
acpi_power->cpu = cpu;
processor_powers[cpu] = acpi_power;
}
- if ( cpu == 0 )
- {
- if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
- {
- get_tick = get_stime_tick;
- ticks_elapsed = stime_ticks_elapsed;
- tick_to_ns = stime_tick_to_ns;
- ns_to_tick = ns_to_stime_tick;
- }
- else
- {
- get_tick = get_acpi_pm_tick;
- ticks_elapsed = acpi_pm_ticks_elapsed;
- tick_to_ns = acpi_pm_tick_to_ns;
- ns_to_tick = ns_to_acpi_pm_tick;
- }
- }
+ acpi_power->count = 2;
+ acpi_power->states[1].type = ACPI_STATE_C1;
+ acpi_power->states[1].entry_method = ACPI_CSTATE_EM_HALT;
+ acpi_power->safe_state = &acpi_power->states[1];
return 0;
}
@@ -885,17 +875,25 @@ static void set_cx(
if ( check_cx(acpi_power, xen_cx) != 0 )
return;
- if ( xen_cx->type == ACPI_STATE_C1 )
+ switch ( xen_cx->type )
+ {
+ case ACPI_STATE_C1:
cx = &acpi_power->states[1];
- else
- cx = &acpi_power->states[acpi_power->count];
-
- if ( !cx->valid )
- acpi_power->count++;
+ break;
+ default:
+ if ( acpi_power->count >= ACPI_PROCESSOR_MAX_POWER )
+ {
+ case ACPI_STATE_C0:
+ printk(XENLOG_WARNING "CPU%u: C%d data ignored\n",
+ acpi_power->cpu, xen_cx->type);
+ return;
+ }
+ cx = &acpi_power->states[acpi_power->count++];
+ cx->type = xen_cx->type;
+ break;
+ }
- cx->valid = 1;
- cx->type = xen_cx->type;
- cx->address = xen_cx->reg.address;
+ cx->address = xen_cx->reg.address;
switch ( xen_cx->reg.space_id )
{
--- 2012-03-06.orig/xen/include/xen/cpuidle.h 2011-06-16 09:21:02.000000000
+0200
+++ 2012-03-06/xen/include/xen/cpuidle.h 2012-03-08 12:34:26.000000000 +0100
@@ -40,7 +40,6 @@
struct acpi_processor_cx
{
u8 idx;
- u8 valid;
u8 type;
u32 address;
u8 entry_method; /* ACPI_CSTATE_EM_xxx */