On Mon, 2022-11-28 at 07:31 +0300, Dan Carpenter wrote:
> Hello Maxim Levitsky,
>
> The patch 92e7d5c83aff: "KVM: x86: allow L1 to not intercept triple
> fault" from Nov 3, 2022, leads to the following Smatch static checker
> warning:
>
> arch/x86/kvm/x86.c:10873 vcpu_enter_guest()
> error: uninitialized symbol 'r'.
>
> arch/x86/kvm/x86.c
> 10509 static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> 10510 {
> 10511 int r;
> 10512 bool req_int_win =
> 10513 dm_request_for_irq_injection(vcpu) &&
> 10514 kvm_cpu_accept_dm_intr(vcpu);
> 10515 fastpath_t exit_fastpath;
> 10516
> 10517 bool req_immediate_exit = false;
> 10518
> 10519 if (kvm_request_pending(vcpu)) {
> 10520 if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) {
> 10521 r = -EIO;
> 10522 goto out;
> 10523 }
> 10524
> 10525 if (kvm_dirty_ring_check_request(vcpu)) {
> 10526 r = 0;
> 10527 goto out;
> 10528 }
> 10529
> 10530 if (kvm_check_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu)) {
> 10531 if (unlikely(!kvm_x86_ops.nested_ops->get_nested_state_pages(vcpu))) {
> 10532 r = 0;
> 10533 goto out;
> 10534 }
> 10535 }
> 10536 if (kvm_check_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu))
> 10537 kvm_mmu_free_obsolete_roots(vcpu);
> 10538 if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
> 10539 __kvm_migrate_timers(vcpu);
> 10540 if (kvm_check_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu))
> 10541 kvm_update_masterclock(vcpu->kvm);
> 10542 if (kvm_check_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu))
> 10543 kvm_gen_kvmclock_update(vcpu);
> 10544 if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, vcpu)) {
> 10545 r = kvm_guest_time_update(vcpu);
> 10546 if (unlikely(r))
> 10547 goto out;
> 10548 }
> 10549 if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
> 10550 kvm_mmu_sync_roots(vcpu);
> 10551 if (kvm_check_request(KVM_REQ_LOAD_MMU_PGD, vcpu))
> 10552 kvm_mmu_load_pgd(vcpu);
> 10553 if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
> 10554 kvm_vcpu_flush_tlb_all(vcpu);
> 10555
> 10556 /* Flushing all ASIDs flushes the current ASID... */
> 10557 kvm_clear_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> 10558 }
> 10559 kvm_service_local_tlb_flush_requests(vcpu);
> 10560
> 10561 if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
> 10562 vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
> 10563 r = 0;
> 10564 goto out;
> 10565 }
> 10566 if (kvm_test_request(KVM_REQ_TRIPLE_FAULT, vcpu)) {
> 10567 if (is_guest_mode(vcpu))
> 10568 kvm_x86_ops.nested_ops->triple_fault(vcpu);
> 10569
> 10570 if (kvm_check_request(KVM_REQ_TRIPLE_FAULT, vcpu)) {
> 10571 vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> 10572 vcpu->mmio_needed = 0;
> 10573 r = 0;
> 10574 }
>
> "r" not initialized on else path. Forgetting to set the error code is
> the canonical bug for do-nothing gotos.
Thank you, patch is on the way!
Best regards,
Maxim Levitsky
>
> 10575 goto out;
> 10576 }
> 10577 if (kvm_check_request(KVM_REQ_APF_HALT, vcpu)) {
> 10578 /* Page is swapped out. Do synthetic halt */
> 10579 vcpu->arch.apf.halted = true;
> 10580 r = 1;
> 10581 goto out;
> 10582 }
> 10583 if (kvm_check_request(KVM_REQ_STEAL_UPDATE, vcpu))
> 10584 record_steal_time(vcpu);
> 10585 if (kvm_check_request(KVM_REQ_SMI, vcpu))
> 10586 process_smi(vcpu);
> 10587 if (kvm_check_request(KVM_REQ_NMI, vcpu))
> 10588 process_nmi(vcpu);
> 10589 if (kvm_check_request(KVM_REQ_PMU, vcpu))
> 10590 kvm_pmu_handle_event(vcpu);
> 10591 if (kvm_check_request(KVM_REQ_PMI, vcpu))
> 10592 kvm_pmu_deliver_pmi(vcpu);
> 10593 if (kvm_check_request(KVM_REQ_IOAPIC_EOI_EXIT, vcpu)) {
> 10594 BUG_ON(vcpu->arch.pending_ioapic_eoi > 255);
> 10595 if (test_bit(vcpu->arch.pending_ioapic_eoi,
> 10596 vcpu->arch.ioapic_handled_vectors)) {
> 10597 vcpu->run->exit_reason = KVM_EXIT_IOAPIC_EOI;
> 10598 vcpu->run->eoi.vector =
> 10599 vcpu->arch.pending_ioapic_eoi;
> 10600 r = 0;
> 10601 goto out;
> 10602 }
> 10603 }
> 10604 if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
> 10605 vcpu_scan_ioapic(vcpu);
> 10606 if (kvm_check_request(KVM_REQ_LOAD_EOI_EXITMAP, vcpu))
> 10607 vcpu_load_eoi_exitmap(vcpu);
> 10608 if (kvm_check_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu))
> 10609 kvm_vcpu_reload_apic_access_page(vcpu);
> 10610 if (kvm_check_request(KVM_REQ_HV_CRASH, vcpu)) {
> 10611 vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
> 10612 vcpu->run->system_event.type = KVM_SYSTEM_EVENT_CRASH;
> 10613 vcpu->run->system_event.ndata = 0;
> 10614 r = 0;
> 10615 goto out;
> 10616 }
> 10617 if (kvm_check_request(KVM_REQ_HV_RESET, vcpu)) {
> 10618 vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
> 10619 vcpu->run->system_event.type = KVM_SYSTEM_EVENT_RESET;
> 10620 vcpu->run->system_event.ndata = 0;
> 10621 r = 0;
> 10622 goto out;
> 10623 }
> 10624 if (kvm_check_request(KVM_REQ_HV_EXIT, vcpu)) {
> 10625 struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
> 10626
> 10627 vcpu->run->exit_reason = KVM_EXIT_HYPERV;
> 10628 vcpu->run->hyperv = hv_vcpu->exit;
> 10629 r = 0;
> 10630 goto out;
> 10631 }
> 10632
> 10633 /*
> 10634 * KVM_REQ_HV_STIMER has to be processed after
> 10635 * KVM_REQ_CLOCK_UPDATE, because Hyper-V SynIC timers
> 10636 * depend on the guest clock being up-to-date
> 10637 */
> 10638 if (kvm_check_request(KVM_REQ_HV_STIMER, vcpu))
> 10639 kvm_hv_process_stimers(vcpu);
> 10640 if (kvm_check_request(KVM_REQ_APICV_UPDATE, vcpu))
> 10641 kvm_vcpu_update_apicv(vcpu);
> 10642 if (kvm_check_request(KVM_REQ_APF_READY, vcpu))
> 10643 kvm_check_async_pf_completion(vcpu);
> 10644 if (kvm_check_request(KVM_REQ_MSR_FILTER_CHANGED, vcpu))
> 10645 static_call(kvm_x86_msr_filter_changed)(vcpu);
> 10646
> 10647 if (kvm_check_request(KVM_REQ_UPDATE_CPU_DIRTY_LOGGING, vcpu))
> 10648 static_call(kvm_x86_update_cpu_dirty_logging)(vcpu);
> 10649 }
> 10650
> 10651 if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win ||
> 10652 kvm_xen_has_interrupt(vcpu)) {
> 10653 ++vcpu->stat.req_event;
> 10654 r = kvm_apic_accept_events(vcpu);
> 10655 if (r < 0) {
> 10656 r = 0;
> 10657 goto out;
> 10658 }
> 10659 if (vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> 10660 r = 1;
> 10661 goto out;
> 10662 }
> 10663
> 10664 r = kvm_check_and_inject_events(vcpu, &req_immediate_exit);
> 10665 if (r < 0) {
> 10666 r = 0;
> 10667 goto out;
> 10668 }
> 10669 if (req_int_win)
> 10670 static_call(kvm_x86_enable_irq_window)(vcpu);
> 10671
> 10672 if (kvm_lapic_enabled(vcpu)) {
> 10673 update_cr8_intercept(vcpu);
> 10674 kvm_lapic_sync_to_vapic(vcpu);
> 10675 }
> 10676 }
> 10677
> 10678 r = kvm_mmu_reload(vcpu);
> 10679 if (unlikely(r)) {
> 10680 goto cancel_injection;
> 10681 }
> 10682
> 10683 preempt_disable();
> 10684
> 10685 static_call(kvm_x86_prepare_switch_to_guest)(vcpu);
> 10686
> 10687 /*
> 10688 * Disable IRQs before setting IN_GUEST_MODE. Posted interrupt
> 10689 * IPI are then delayed after guest entry, which ensures that they
> 10690 * result in virtual interrupt delivery.
> 10691 */
> 10692 local_irq_disable();
> 10693
> 10694 /* Store vcpu->apicv_active before vcpu->mode. */
> 10695 smp_store_release(&vcpu->mode, IN_GUEST_MODE);
> 10696
> 10697 kvm_vcpu_srcu_read_unlock(vcpu);
> 10698
> 10699 /*
> 10700 * 1) We should set ->mode before checking ->requests. Please see
> 10701 * the comment in kvm_vcpu_exiting_guest_mode().
> 10702 *
> 10703 * 2) For APICv, we should set ->mode before checking PID.ON. This
> 10704 * pairs with the memory barrier implicit in pi_test_and_set_on
> 10705 * (see vmx_deliver_posted_interrupt).
> 10706 *
> 10707 * 3) This also orders the write to mode from any reads to the page
> 10708 * tables done while the VCPU is running. Please see the comment
> 10709 * in kvm_flush_remote_tlbs.
> 10710 */
> 10711 smp_mb__after_srcu_read_unlock();
> 10712
> 10713 /*
> 10714 * Process pending posted interrupts to handle the case where the
> 10715 * notification IRQ arrived in the host, or was never sent (because the
> 10716 * target vCPU wasn't running). Do this regardless of the vCPU's APICv
> 10717 * status, KVM doesn't update assigned devices when APICv is inhibited,
> 10718 * i.e. they can post interrupts even if APICv is temporarily disabled.
> 10719 */
> 10720 if (kvm_lapic_enabled(vcpu))
> 10721 static_call_cond(kvm_x86_sync_pir_to_irr)(vcpu);
> 10722
> 10723 if (kvm_vcpu_exit_request(vcpu)) {
> 10724 vcpu->mode = OUTSIDE_GUEST_MODE;
> 10725 smp_wmb();
> 10726 local_irq_enable();
> 10727 preempt_enable();
> 10728 kvm_vcpu_srcu_read_lock(vcpu);
> 10729 r = 1;
> 10730 goto cancel_injection;
> 10731 }
> 10732
> 10733 if (req_immediate_exit) {
> 10734 kvm_make_request(KVM_REQ_EVENT, vcpu);
> 10735 static_call(kvm_x86_request_immediate_exit)(vcpu);
> 10736 }
> 10737
> 10738 fpregs_assert_state_consistent();
> 10739 if (test_thread_flag(TIF_NEED_FPU_LOAD))
> 10740 switch_fpu_return();
> 10741
> 10742 if (vcpu->arch.guest_fpu.xfd_err)
> 10743 wrmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err);
> 10744
> 10745 if (unlikely(vcpu->arch.switch_db_regs)) {
> 10746 set_debugreg(0, 7);
> 10747 set_debugreg(vcpu->arch.eff_db[0], 0);
> 10748 set_debugreg(vcpu->arch.eff_db[1], 1);
> 10749 set_debugreg(vcpu->arch.eff_db[2], 2);
> 10750 set_debugreg(vcpu->arch.eff_db[3], 3);
> 10751 } else if (unlikely(hw_breakpoint_active())) {
> 10752 set_debugreg(0, 7);
> 10753 }
> 10754
> 10755 guest_timing_enter_irqoff();
> 10756
> 10757 for (;;) {
> 10758 /*
> 10759 * Assert that vCPU vs. VM APICv state is consistent. An APICv
> 10760 * update must kick and wait for all vCPUs before toggling the
> 10761 * per-VM state, and responsing vCPUs must wait for the update
> 10762 * to complete before servicing KVM_REQ_APICV_UPDATE.
> 10763 */
> 10764 WARN_ON_ONCE((kvm_vcpu_apicv_activated(vcpu) != kvm_vcpu_apicv_active(vcpu)) &&
> 10765 (kvm_get_apic_mode(vcpu) != LAPIC_MODE_DISABLED));
> 10766
> 10767 exit_fastpath = static_call(kvm_x86_vcpu_run)(vcpu);
> 10768 if (likely(exit_fastpath != EXIT_FASTPATH_REENTER_GUEST))
> 10769 break;
> 10770
> 10771 if (kvm_lapic_enabled(vcpu))
> 10772 static_call_cond(kvm_x86_sync_pir_to_irr)(vcpu);
> 10773
> 10774 if (unlikely(kvm_vcpu_exit_request(vcpu))) {
> 10775 exit_fastpath = EXIT_FASTPATH_EXIT_HANDLED;
> 10776 break;
> 10777 }
> 10778 }
> 10779
> 10780 /*
> 10781 * Do this here before restoring debug registers on the host. And
> 10782 * since we do this before handling the vmexit, a DR access vmexit
> 10783 * can (a) read the correct value of the debug registers, (b) set
> 10784 * KVM_DEBUGREG_WONT_EXIT again.
> 10785 */
> 10786 if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) {
> 10787 WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);
> 10788 static_call(kvm_x86_sync_dirty_debug_regs)(vcpu);
> 10789 kvm_update_dr0123(vcpu);
> 10790 kvm_update_dr7(vcpu);
> 10791 }
> 10792
> 10793 /*
> 10794 * If the guest has used debug registers, at least dr7
> 10795 * will be disabled while returning to the host.
> 10796 * If we don't have active breakpoints in the host, we don't
> 10797 * care about the messed up debug address registers. But if
> 10798 * we have some of them active, restore the old state.
> 10799 */
> 10800 if (hw_breakpoint_active())
> 10801 hw_breakpoint_restore();
> 10802
> 10803 vcpu->arch.last_vmentry_cpu = vcpu->cpu;
> 10804 vcpu->arch.last_guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
> 10805
> 10806 vcpu->mode = OUTSIDE_GUEST_MODE;
> 10807 smp_wmb();
> 10808
> 10809 /*
> 10810 * Sync xfd before calling handle_exit_irqoff() which may
> 10811 * rely on the fact that guest_fpu::xfd is up-to-date (e.g.
> 10812 * in #NM irqoff handler).
> 10813 */
> 10814 if (vcpu->arch.xfd_no_write_intercept)
> 10815 fpu_sync_guest_vmexit_xfd_state();
> 10816
> 10817 static_call(kvm_x86_handle_exit_irqoff)(vcpu);
> 10818
> 10819 if (vcpu->arch.guest_fpu.xfd_err)
> 10820 wrmsrl(MSR_IA32_XFD_ERR, 0);
> 10821
> 10822 /*
> 10823 * Consume any pending interrupts, including the possible source of
> 10824 * VM-Exit on SVM and any ticks that occur between VM-Exit and now.
> 10825 * An instruction is required after local_irq_enable() to fully unblock
> 10826 * interrupts on processors that implement an interrupt shadow, the
> 10827 * stat.exits increment will do nicely.
> 10828 */
> 10829 kvm_before_interrupt(vcpu, KVM_HANDLING_IRQ);
> 10830 local_irq_enable();
> 10831 ++vcpu->stat.exits;
> 10832 local_irq_disable();
> 10833 kvm_after_interrupt(vcpu);
> 10834
> 10835 /*
> 10836 * Wait until after servicing IRQs to account guest time so that any
> 10837 * ticks that occurred while running the guest are properly accounted
> 10838 * to the guest. Waiting until IRQs are enabled degrades the accuracy
> 10839 * of accounting via context tracking, but the loss of accuracy is
> 10840 * acceptable for all known use cases.
> 10841 */
> 10842 guest_timing_exit_irqoff();
> 10843
> 10844 local_irq_enable();
> 10845 preempt_enable();
> 10846
> 10847 kvm_vcpu_srcu_read_lock(vcpu);
> 10848
> 10849 /*
> 10850 * Profile KVM exit RIPs:
> 10851 */
> 10852 if (unlikely(prof_on == KVM_PROFILING)) {
> 10853 unsigned long rip = kvm_rip_read(vcpu);
> 10854 profile_hit(KVM_PROFILING, (void *)rip);
> 10855 }
> 10856
> 10857 if (unlikely(vcpu->arch.tsc_always_catchup))
> 10858 kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
> 10859
> 10860 if (vcpu->arch.apic_attention)
> 10861 kvm_lapic_sync_from_vapic(vcpu);
> 10862
> 10863 r = static_call(kvm_x86_handle_exit)(vcpu, exit_fastpath);
> 10864 return r;
> 10865
> 10866 cancel_injection:
> 10867 if (req_immediate_exit)
> 10868 kvm_make_request(KVM_REQ_EVENT, vcpu);
> 10869 static_call(kvm_x86_cancel_injection)(vcpu);
> 10870 if (unlikely(vcpu->arch.apic_attention))
> 10871 kvm_lapic_sync_from_vapic(vcpu);
> 10872 out:
> --> 10873 return r;
> 10874 }
>
> regards,
> dan carpenter
>
