Re: [Xen-devel] [PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.03.12 at 01:39, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> 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@xxxxxxxxxxxxx 
>> [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of eric
>> Sent: Wednesday, March 07, 2012 10:07 PM
>> To: xen-devel@xxxxxxxxxxxxx 
>> Cc: julian.pidancet@xxxxxxxxxxxxx 
>> 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@xxxxxxxxxxxxx>
>> 
>> 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@xxxxxxxxxxxxx 
>> http://lists.xen.org/xen-devel 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel