On 7/19/24 22:14, Jim Mattson wrote: > On Fri, Jul 19, 2024 at 12:41 PM Mirsad Todorovac > <mtodorovac69@xxxxxxxxx> wrote: >> >> >> >> 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. :-/ > > Comments would also help. :) > >> 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. :-) > > I assure you that there are plenty of actual bugs in KVM. This tool > just isn't finding them. Well, this series of reports did not target KVM. It was accidental that GCC static analyser reported those dubious false positives first. Best regards, Mirsad Todorovac >> Sort of cool that you guys on Google consider bug report from nobody admins from the >> universities ;-) >> >> Best regards, >> Mirsad Todorovac >>
