Re: [PATCH] sched, x86: fix overflow in cyc2ns_offset

[john stultz <johnstul@us.ibm.com>]

On Thu, 2012-03-08 at 15:23 -0800, Salman Qazi wrote:
> When a machine boots up, the TSC generally gets reset.  However, when
> kexec is used to boot into a kernel, the TSC value would be carried
> over from the previous kernel.  The computation of cycns_offset in
> set_cyc2ns_scale is prone to an overflow, if the machine has been up
> more than 208 days prior to the kexec.  The overflow happens when
> we multiply *scale, even though there is enough room to store the
> final answer.  We fix this issue by decomposing tsc_now into the
> quotient and remainder of division by CYC2NS_SCALE_FACTOR and then
> performing the multiplication separately on the two components.
> 
> Signed-off-by: Salman Qazi <sqazi@google.com>
> ---
>  arch/x86/kernel/tsc.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index a62c201..ef1dc8e 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -608,6 +608,8 @@ static void set_cyc2ns_scale(unsigned long cpu_khz, int cpu)
>  {
>  	unsigned long long tsc_now, ns_now, *offset;
>  	unsigned long flags, *scale;
> +	unsigned long long quot;
> +	unsigned long long rem;
> 
>  	local_irq_save(flags);
>  	sched_clock_idle_sleep_event();
> @@ -620,7 +622,15 @@ static void set_cyc2ns_scale(unsigned long cpu_khz, int cpu)
> 
>  	if (cpu_khz) {
>  		*scale = (NSEC_PER_MSEC << CYC2NS_SCALE_FACTOR)/cpu_khz;
> -		*offset = ns_now - (tsc_now * *scale >> CYC2NS_SCALE_FACTOR);
> +
> +		/*
> +		 * Avoid premature overflow by splitting into quotient
> +		 * and remainder.  See the comment above __cycles_2_ns
> +		 */
> +		quot = (tsc_now >> CYC2NS_SCALE_FACTOR);
> +		rem = tsc_now & ((1ULL << CYC2NS_SCALE_FACTOR) - 1);
> +		*offset = ns_now - (quot * *scale +
> +				    ((rem * *scale) >> CYC2NS_SCALE_FACTOR));
>  	}

This clearly is a needed fix. Thanks for finding it and sending it in.

Although I'm curious if it might be good to encapsulate this code into a
macro that can be reused in both set_cyc2ns_scale and __cycles_2_ns()
(as well as others, I suspect this issue is going to crop up on other
arches at some point too)?

thanks
-john