| Message ID | 20221020113759.17402-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/xen: silence smatch warning in pmu_msr_chk_emulated() | expand |
On 20.10.2022 13:37, Juergen Gross wrote: > Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr > accesses") introduced code resulting in a warning issued by the smatch > static checker, claiming to use an uninitialized variable. > > This is a false positive, but work around the warning nevertheless. The risk of introducing a problem might be quite low here, but in general it exists: With the adjustment you remove any chance of the compiler spotting a missing initialization before use. And I'm not convinced using 0 in such a case would actually be ending up sufficiently benign. Jan > --- a/arch/x86/xen/pmu.c > +++ b/arch/x86/xen/pmu.c > @@ -302,7 +302,7 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read) > static bool pmu_msr_chk_emulated(unsigned int msr, uint64_t *val, bool is_read, > bool *emul) > { > - int type, index; > + int type = 0, index = 0; > > if (is_amd_pmu_msr(msr)) > *emul = xen_amd_pmu_emulate(msr, val, is_read);
On 20.10.22 15:16, Jan Beulich wrote: > On 20.10.2022 13:37, Juergen Gross wrote: >> Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr >> accesses") introduced code resulting in a warning issued by the smatch >> static checker, claiming to use an uninitialized variable. >> >> This is a false positive, but work around the warning nevertheless. > > The risk of introducing a problem might be quite low here, but in general > it exists: With the adjustment you remove any chance of the compiler > spotting a missing initialization before use. And I'm not convinced using > 0 in such a case would actually be ending up sufficiently benign. Hmm, an alternative would be to initialize it to -1 and add a test for the index to be >= 0 before using it. Or to live with the smash warning with the chance, that a compiler might be warning for the same reason in the future. Juergen
On 10/20/22 9:34 AM, Juergen Gross wrote: > On 20.10.22 15:16, Jan Beulich wrote: >> On 20.10.2022 13:37, Juergen Gross wrote: >>> Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr >>> accesses") introduced code resulting in a warning issued by the smatch >>> static checker, claiming to use an uninitialized variable. >>> >>> This is a false positive, but work around the warning nevertheless. >> >> The risk of introducing a problem might be quite low here, but in general >> it exists: With the adjustment you remove any chance of the compiler >> spotting a missing initialization before use. And I'm not convinced using >> 0 in such a case would actually be ending up sufficiently benign. > > Hmm, an alternative would be to initialize it to -1 and add a test for the > index to be >= 0 before using it. > > Or to live with the smash warning with the chance, that a compiler might be > warning for the same reason in the future. Is smatch complaining about both variables or just index? There are two cases in is_intel_pmu_msr() where it returns true but index is not set so perhaps that's what bothers smatch? It shold not complain if is_intel_pmu_msr() returns false. -boris
On 20.10.22 16:22, Boris Ostrovsky wrote: > > On 10/20/22 9:34 AM, Juergen Gross wrote: >> On 20.10.22 15:16, Jan Beulich wrote: >>> On 20.10.2022 13:37, Juergen Gross wrote: >>>> Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr >>>> accesses") introduced code resulting in a warning issued by the smatch >>>> static checker, claiming to use an uninitialized variable. >>>> >>>> This is a false positive, but work around the warning nevertheless. >>> >>> The risk of introducing a problem might be quite low here, but in general >>> it exists: With the adjustment you remove any chance of the compiler >>> spotting a missing initialization before use. And I'm not convinced using >>> 0 in such a case would actually be ending up sufficiently benign. >> >> Hmm, an alternative would be to initialize it to -1 and add a test for the >> index to be >= 0 before using it. >> >> Or to live with the smash warning with the chance, that a compiler might be >> warning for the same reason in the future. > > > Is smatch complaining about both variables or just index? There are two cases in > is_intel_pmu_msr() where it returns true but index is not set so perhaps that's > what bothers smatch? It shold not complain if is_intel_pmu_msr() returns false. I didn't test it myself, so I can only speculate. I guess the problem is when is_intel_pmu_msr() returns true. In the end I don't think we expect much code churn in this area in the future. Its not as if the pmu handling for PV guests is expected to be extended. Juergen
On Thu, Oct 20, 2022 at 10:22:17AM -0400, Boris Ostrovsky wrote: > > On 10/20/22 9:34 AM, Juergen Gross wrote: > > On 20.10.22 15:16, Jan Beulich wrote: > > > On 20.10.2022 13:37, Juergen Gross wrote: > > > > Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr > > > > accesses") introduced code resulting in a warning issued by the smatch > > > > static checker, claiming to use an uninitialized variable. > > > > > > > > This is a false positive, but work around the warning nevertheless. > > > > > > The risk of introducing a problem might be quite low here, but in general > > > it exists: With the adjustment you remove any chance of the compiler > > > spotting a missing initialization before use. And I'm not convinced using > > > 0 in such a case would actually be ending up sufficiently benign. > > > > Hmm, an alternative would be to initialize it to -1 and add a test for the > > index to be >= 0 before using it. > > > > Or to live with the smash warning with the chance, that a compiler might be > > warning for the same reason in the future. > > > Is smatch complaining about both variables or just index? Just "index". > There are two cases in is_intel_pmu_msr() where it returns true but > index is not set so perhaps that's what bothers smatch? Yep. The "index" variable *is* undefined when it's passed so Smatch is correct in what it's saying. But it's is not used on that path inside the function so it's harmless. > It shold not complain if is_intel_pmu_msr() returns false. Correct. I kind of like the patch. We generally say "fix the checker and don't silence the warning" but in this case I feel like the checker is doing the best possible thing and I'm not going to fix it. Trying to silence this warning in Smatch would come with some real downsides. regards, dan carpenter
diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c index 68aff1382872..898a252ed6f1 100644 --- a/arch/x86/xen/pmu.c +++ b/arch/x86/xen/pmu.c @@ -302,7 +302,7 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read) static bool pmu_msr_chk_emulated(unsigned int msr, uint64_t *val, bool is_read, bool *emul) { - int type, index; + int type = 0, index = 0; if (is_amd_pmu_msr(msr)) *emul = xen_amd_pmu_emulate(msr, val, is_read);
Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr accesses") introduced code resulting in a warning issued by the smatch static checker, claiming to use an uninitialized variable. This is a false positive, but work around the warning nevertheless. Fixes: 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr accesses") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- arch/x86/xen/pmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)