On 7/19/24 21:22, Sean Christopherson wrote: > On Fri, Jul 19, 2024, Mirsad Todorovac wrote: >> On 7/19/24 19:21, Sean Christopherson wrote: >>> On Fri, Jul 19, 2024, Mirsad Todorovac wrote: >>>> Hi, >>>> >>>> In the build of 6.10.0 from stable tree, the following error was detected. >>>> >>>> You see that the function get_fixed_pmc() can return NULL pointer as a result >>>> if msr is outside of [base, base + pmu->nr_arch_fixed_counters) interval. >>>> >>>> kvm_pmu_request_counter_reprogram(pmc) is then called with that NULL pointer >>>> as the argument, which expands to .../pmu.h >>>> >>>> #define pmc_to_pmu(pmc) (&(pmc)->vcpu->arch.pmu) >>>> >>>> which is a NULL pointer dereference in that speculative case. >>> >>> I'm somewhat confused. Did you actually hit a BUG() due to a NULL-pointer >>> dereference, are you speculating that there's a bug, or did you find some speculation >>> issue with the CPU? >>> >>> It should be impossible for get_fixed_pmc() to return NULL in this case. The >>> loop iteration is fully controlled by KVM, i.e. 'i' is guaranteed to be in the >>> ranage [0..pmu->nr_arch_fixed_counters). >>> >>> And the input @msr is "MSR_CORE_PERF_FIXED_CTR0 +i", so the if-statement expands to: >>> >>> if (MSR_CORE_PERF_FIXED_CTR0 + [0..pmu->nr_arch_fixed_counters) >= MSR_CORE_PERF_FIXED_CTR0 && >>> MSR_CORE_PERF_FIXED_CTR0 + [0..pmu->nr_arch_fixed_counters) < MSR_CORE_PERF_FIXED_CTR0 + pmu->nr_arch_fixed_counters) >>> >>> i.e. is guaranteed to evaluate true. >>> >>> Am I missing something? >> >> Hi Sean, >> >> Thank you for replying promptly. >> >> Perhaps I should have provided the GCC error report in the first place. > > Yes, though the report itself is somewhat secondary, what matters the most is how > you found the bug and how to reproduce the failure. Critically, IIUC, this requires > analyzer-null-dereference, which AFAIK isn't even enabled by W=1, let alone a base > build. > > Please see the 0-day bot's reports[*] for a fantastic example of how to report > things that are found by non-standard (by kernel standards) means. > > In general, I suspect that analyzer-null-dereference will generate a _lot_ of > false positives, and is probably not worth reporting unless you are absolutely > 100% certain there's a real bug. I (and most maintainers) am happy to deal with > false positives here and there _if_ the signal to noise ratio is high. But if > most reports are false positives, they'll likely all end up getting ignored. > > [*] https://lore.kernel.org/all/202406111250.d8XtA9SC-lkp@xxxxxxxxx I think I understood the meaning between the lines. However, to repeat the obvious, reducing the global dependencies simplifies the readability and the logical proof of the code. :-/ Needless to say, dividing into pure functions and const functions reduces the number of dependencies, as it is N × (N - 1), sqr (N). For example, if a condition is always true, but the compiler cannot deduce it from code, there is something odd. CONCLUSION: If this generated 5 out of 5 false positives, then I might be giving up on this as a waste of your time. However, it was great fun analysing x86 KVM code. :-) Sort of cool that you guys on Google consider bug report from nobody admins from the universities ;-) Best regards, Mirsad Todorovac
