On Thu, 2022-05-05 at 10:05 +0300, Dan Carpenter wrote:
> Hello Giovanni Gherdovich,
>
> The patch 51beea8862a3: "x86, sched: Bail out of frequency invariance
> if turbo frequency is unknown" from May 31, 2020, leads to the
> following Smatch static checker warning:
>
> arch/x86/kernel/cpu/aperfmperf.c:274 intel_set_max_freq_ratio()
> error: uninitialized symbol 'turbo_freq'.
>
> arch/x86/kernel/cpu/aperfmperf.c
> 242 static bool __init intel_set_max_freq_ratio(void)
> 243 {
> 244 u64 base_freq, turbo_freq;
> 245 u64 turbo_ratio;
> 246
> 247 if (slv_set_max_freq_ratio(&base_freq, &turbo_freq))
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Imagine this fails.
>
> 248 goto out;
I think this is a false positive of the static checker; I'll send the patch
to initialize the variables anyway as it looks better.
When slv_set_max_freq_ratio() fails, it returns false, and we move on to the
next "if" statement. "goto out" is executed only if slv_set_max_freq_ratio()
succeeds (returns true), and writes data in its input parameters.
I give you that the code looks wrong, because apparently when people (and
static analyzers) read that, they think "if (problem) goto error;" but this
function is written like "if (it_works) goto the_end;".
When I received your report I thought the code was wrong, too.
You already sent me this report two years ago,
https://www.spinics.net/lists/linux-kernel-janitors/msg51372.html
and I didn't reply (I should have).
> 249
> 250 if (x86_match_cpu(has_glm_turbo_ratio_limits) &&
> 251 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
> 252 goto out;
> 253
> 254 if (x86_match_cpu(has_knl_turbo_ratio_limits) &&
> 255 knl_set_max_freq_ratio(&base_freq, &turbo_freq, 1))
> 256 goto out;
> 257
> 258 if (x86_match_cpu(has_skx_turbo_ratio_limits) &&
> 259 skx_set_max_freq_ratio(&base_freq, &turbo_freq, 4))
> 260 goto out;
> 261
> 262 if (core_set_max_freq_ratio(&base_freq, &turbo_freq))
> 263 goto out;
> 264
> 265 return false;
The "return false" above is what happens if none of the "if" matched, and this
is when base_freq and turbo_freq are uninitialized.
> 266
> 267 out:
> 268 /*
> 269 * Some hypervisors advertise X86_FEATURE_APERFMPERF
> 270 * but then fill all MSR's with zeroes.
> 271 * Some CPUs have turbo boost but don't declare any turbo ratio
> 272 * in MSR_TURBO_RATIO_LIMIT.
> 273 */
> --> 274 if (!base_freq || !turbo_freq) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> Uninitialized.
I know it looks sketchy (and makes you think I forgot the initialization), but
the condition here is checking for when a *_set_max_freq_ratio() above
matched, but received zeroes from reading the corresponding MSR.
Anyways, patch incoming to make this look better, but I don't think it's a
"fixes: [...]".
> Although I notice that base_freq is also unintialized
> and that predates your patch... So I should probably send this bug
> report to someone else... Sorry?
>
> 275 pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n");
> 276 return false;
> 277 }
> 278
> 279 turbo_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, base_freq);
> 280 if (!turbo_ratio) {
> 281 pr_debug("Non-zero turbo and base frequencies led to a 0 ratio.\n");
> 282 return false;
> 283 }
> 284
> 285 arch_turbo_freq_ratio = turbo_ratio;
> 286 arch_set_max_freq_ratio(turbo_disabled());
> 287
> 288 return true;
> 289 }
>
> regards,
> dan carpenter
