fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

[Christoph Hellwig]

Just notice the following error from gcc 4.4:

arch/x86/kernel/tsc.c: In function 'time_cpufreq_notifier':
arch/x86/kernel/tsc.c:634: warning: 'dummy' may be used uninitialized in this function

dummy is only used in the following way in this function:

	lpj = &dummy;

and then dummy might be overriden in the following odd way:

	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
#ifdef CONFIG_SMP
		lpj = &cpu_data(freq->cpu).loops_per_jiffy;
#else
	lpj = &boot_cpu_data.loops_per_jiffy;
#endif

and then is used in

	if (!ref_freq) {
		ref_freq = freq->old;
		loops_per_jiffy_ref = *lpj;
		tsc_khz_ref = tsc_khz;
	}

to me that looks like it can indeed be used unitialized for the case
where we do have CONFIG_SMP set, freq->flags & CPUFREQ_CONST_LOOPS is
true and ref_freq is false.

Can that case actually happen?

Re: fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

[Dave Jones]

On Mon, Jun 01, 2009 at 10:21:04AM -0400, Christoph Hellwig wrote:
 > Just notice the following error from gcc 4.4:
 > 
 > arch/x86/kernel/tsc.c: In function 'time_cpufreq_notifier':
 > arch/x86/kernel/tsc.c:634: warning: 'dummy' may be used uninitialized in this function
 > 
 > where we do have CONFIG_SMP set, freq->flags & CPUFREQ_CONST_LOOPS is
 > true and ref_freq is false.
 > 
 > Can that case actually happen?
 
I think you're right, though the circumstances for hitting it are really
low. Nearly all SMP capable cpufreq drivers set CPUFREQ_CONST_LOOPS.
powernow-k8 is really the only exception. The older CPUs were typically
only ever UP. (powernow-k7 never supported SMP for eg)

It's probably worth fixing.

I think the patch below is closer to the actual intent, with the bonus
of being a lot more readable.

Sanity check?

	Dave

Fix possible uninitialized use of dummy, by just removing it,
and making the setting of lpj more obvious.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index d57de05..78c54ea 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 				void *data)
 {
 	struct cpufreq_freqs *freq = data;
-	unsigned long *lpj, dummy;
+	unsigned long *lpj;
 
 	if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
 		return 0;
 
-	lpj = &dummy;
-	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
+	lpj = &boot_cpu_data.loops_per_jiffy;
 #ifdef CONFIG_SMP
+	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
 		lpj = &cpu_data(freq->cpu).loops_per_jiffy;
-#else
-	lpj = &boot_cpu_data.loops_per_jiffy;
 #endif
 
 	if (!ref_freq) {

Re: fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

[Christoph Hellwig]

On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index d57de05..78c54ea 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
>  				void *data)
>  {
>  	struct cpufreq_freqs *freq = data;
> -	unsigned long *lpj, dummy;
> +	unsigned long *lpj;
>  
>  	if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
>  		return 0;
>  
> -	lpj = &dummy;
> -	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> +	lpj = &boot_cpu_data.loops_per_jiffy;
>  #ifdef CONFIG_SMP
> +	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
>  		lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> -#else
> -	lpj = &boot_cpu_data.loops_per_jiffy;
>  #endif

This makes the code look a lot more sane and should fix the potential
issue.

Re: fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

[Jon Masters]

On Mon, 2009-06-01 at 12:39 -0400, Christoph Hellwig wrote:
> On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index d57de05..78c54ea 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> >  				void *data)
> >  {
> >  	struct cpufreq_freqs *freq = data;
> > -	unsigned long *lpj, dummy;
> > +	unsigned long *lpj;
> >  
> >  	if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
> >  		return 0;
> >  
> > -	lpj = &dummy;
> > -	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > +	lpj = &boot_cpu_data.loops_per_jiffy;
> >  #ifdef CONFIG_SMP
> > +	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> >  		lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> > -#else
> > -	lpj = &boot_cpu_data.loops_per_jiffy;
> >  #endif
> 
> This makes the code look a lot more sane and should fix the potential
> issue.

Tiny niggle that you wind up setting lpj (loops per jiffy) twice if
you're on SMP and have CPUFREQ_CONST_LOOPS.

Jon.

Re: fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

[Michael S. Zick]

On Mon June 1 2009, Jon Masters wrote:
> On Mon, 2009-06-01 at 12:39 -0400, Christoph Hellwig wrote:
> > On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > index d57de05..78c54ea 100644
> > > --- a/arch/x86/kernel/tsc.c
> > > +++ b/arch/x86/kernel/tsc.c
> > > @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> > >  				void *data)
> > >  {
> > >  	struct cpufreq_freqs *freq = data;
> > > -	unsigned long *lpj, dummy;
> > > +	unsigned long *lpj;
> > >  
> > >  	if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
> > >  		return 0;
> > >  
> > > -	lpj = &dummy;
> > > -	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > > +	lpj = &boot_cpu_data.loops_per_jiffy;
> > >  #ifdef CONFIG_SMP
> > > +	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > >  		lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> > > -#else
> > > -	lpj = &boot_cpu_data.loops_per_jiffy;
> > >  #endif
> > 
> > This makes the code look a lot more sane and should fix the potential
> > issue.
> 
> Tiny niggle that you wind up setting lpj (loops per jiffy) twice if
> you're on SMP and have CPUFREQ_CONST_LOOPS.
> 

At least it is consistent. ;)
You set lpj twice if your not on SMP and have CPUFREG_CONST_LOOPS.

Mike
> Jon.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

[Michael S. Zick]

On Mon June 1 2009, Michael S. Zick wrote:

Just ignore the last - I hit the "send" rather than "discard".

Mike
> On Mon June 1 2009, Jon Masters wrote:
> > On Mon, 2009-06-01 at 12:39 -0400, Christoph Hellwig wrote:
> > > On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> > > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > > index d57de05..78c54ea 100644
> > > > --- a/arch/x86/kernel/tsc.c
> > > > +++ b/arch/x86/kernel/tsc.c
> > > > @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> > > >  				void *data)
> > > >  {
> > > >  	struct cpufreq_freqs *freq = data;
> > > > -	unsigned long *lpj, dummy;
> > > > +	unsigned long *lpj;
> > > >  
> > > >  	if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
> > > >  		return 0;
> > > >  
> > > > -	lpj = &dummy;
> > > > -	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > > > +	lpj = &boot_cpu_data.loops_per_jiffy;
> > > >  #ifdef CONFIG_SMP
> > > > +	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > > >  		lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> > > > -#else
> > > > -	lpj = &boot_cpu_data.loops_per_jiffy;
> > > >  #endif
> > > 
> > > This makes the code look a lot more sane and should fix the potential
> > > issue.
> > 
> > Tiny niggle that you wind up setting lpj (loops per jiffy) twice if
> > you're on SMP and have CPUFREQ_CONST_LOOPS.
> > 
> 
> At least it is consistent. ;)
> You set lpj twice if your not on SMP and have CPUFREG_CONST_LOOPS.
> 
> Mike
> > Jon.
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> > 
> 
> 
> 

Re: fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()

[Daniel Barkalow]

On Mon, 1 Jun 2009, Christoph Hellwig wrote:

> Just notice the following error from gcc 4.4:
> 
> arch/x86/kernel/tsc.c: In function 'time_cpufreq_notifier':
> arch/x86/kernel/tsc.c:634: warning: 'dummy' may be used uninitialized in this function
> 
> dummy is only used in the following way in this function:
> 
> 	lpj = &dummy;
> 
> and then dummy might be overriden in the following odd way:
> 
> 	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> #ifdef CONFIG_SMP
> 		lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> #else
> 	lpj = &boot_cpu_data.loops_per_jiffy;
> #endif

This is misindented; the if applies to both CONFIG_SMP and otherwise. For 
that matter, cpu_data(anything) == boot_cpu_data if !CONFIG_SMP. So the 
current code is equivalent to:

	if (!(freq->flags & CPUFREQ_CONST_LOOPS))
		lpj = &cpu_data(freq->cpu).loops_per_jiffy;

> and then is used in
> 
> 	if (!ref_freq) {
> 		ref_freq = freq->old;
> 		loops_per_jiffy_ref = *lpj;
> 		tsc_khz_ref = tsc_khz;
> 	}
> 
> to me that looks like it can indeed be used unitialized for the case
> where we do have CONFIG_SMP set, freq->flags & CPUFREQ_CONST_LOOPS is
> true and ref_freq is false.
> 
> Can that case actually happen?

Looks to me like loops_per_jiffy_ref is only used to compute a new value 
for *lpj. So the case that matters is if this function can be called the 
first time with freq->flags & CPUFREQ_CONST_LOOPS and then without; 
otherwise, the uninitialized values only contribute to a dead assignment 
(and the junk in the static variable).

I'd guess that, if freq->flags & CPUFREQ_CONST_LOOPS, no processor's 
loops_per_jiffy should get scaled, so the current code is essentially 
correct, although it's hard to read and far too hard for the compiler to 
analyze.

Probably the right answer is to move the *lpj = ... in with 
mark_tsc_unstable and drop the earlier if and dummy.

	-Daniel
*This .sig left intentionally blank*