• Sean Christopherson's avatar
    KVM: x86: Serialize vendor module initialization (hardware setup) · 3af4a9e6
    Sean Christopherson authored
    Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while
    doing hardware setup to ensure that concurrent calls are fully serialized.
    KVM rejects attempts to load vendor modules if a different module has
    already been loaded, but doesn't handle the case where multiple vendor
    modules are loaded at the same time, and module_init() doesn't run under
    the global module_mutex.
    
    Note, in practice, this is likely a benign bug as no platform exists that
    supports both SVM and VMX, i.e. barring a weird VM setup, one of the
    vendor modules is guaranteed to fail a support check before modifying
    common KVM state.
    
    Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable,
    but that comes with its own ugliness as it would require setting
    .hardware_enable before success is guaranteed, e.g. attempting to load
    the "wrong" could result in spurious failure to load the "right" module.
    
    Introduce a new mutex as using kvm_lock is extremely deadlock prone due
    to kvm_lock being taken under cpus_write_lock(), and in the future, under
    under cpus_read_lock().  Any operation that takes cpus_read_lock() while
    holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes
    cpus_read_lock() to register a callback.  In theory, KVM could avoid
    such problematic paths, i.e. do less setup under kvm_lock, but avoiding
    all calls to cpus_read_lock() is subtly difficult and thus fragile.  E.g.
    updating static calls also acquires cpus_read_lock().
    
    Inverting the lock ordering, i.e. always taking kvm_lock outside
    cpus_read_lock(), is not a viable option as kvm_lock is taken in various
    callbacks that may be invoked under cpus_read_lock(), e.g. x86's
    kvmclock_cpufreq_notifier().
    
    The lockdep splat below is dependent on future patches to take
    cpus_read_lock() in hardware_enable_all(), but as above, deadlock is
    already is already possible.
    
      ======================================================
      WARNING: possible circular locking dependency detected
      6.0.0-smp--7ec93244f194-init2 #27 Tainted: G           O
      ------------------------------------------------------
      stable/251833 is trying to acquire lock:
      ffffffffc097ea28 (kvm_lock){+.+.}-{3:3}, at: hardware_enable_all+0x1f/0xc0 [kvm]
    
                   but task is already holding lock:
      ffffffffa2456828 (cpu_hotplug_lock){++++}-{0:0}, at: hardware_enable_all+0xf/0xc0 [kvm]
    
                   which lock already depends on the new lock.
    
                   the existing dependency chain (in reverse order) is:
    
                   -> #1 (cpu_hotplug_lock){++++}-{0:0}:
             cpus_read_lock+0x2a/0xa0
             __cpuhp_setup_state+0x2b/0x60
             __kvm_x86_vendor_init+0x16a/0x1870 [kvm]
             kvm_x86_vendor_init+0x23/0x40 [kvm]
             0xffffffffc0a4d02b
             do_one_initcall+0x110/0x200
             do_init_module+0x4f/0x250
             load_module+0x1730/0x18f0
             __se_sys_finit_module+0xca/0x100
             __x64_sys_finit_module+0x1d/0x20
             do_syscall_64+0x3d/0x80
             entry_SYSCALL_64_after_hwframe+0x63/0xcd
    
                   -> #0 (kvm_lock){+.+.}-{3:3}:
             __lock_acquire+0x16f4/0x30d0
             lock_acquire+0xb2/0x190
             __mutex_lock+0x98/0x6f0
             mutex_lock_nested+0x1b/0x20
             hardware_enable_all+0x1f/0xc0 [kvm]
             kvm_dev_ioctl+0x45e/0x930 [kvm]
             __se_sys_ioctl+0x77/0xc0
             __x64_sys_ioctl+0x1d/0x20
             do_syscall_64+0x3d/0x80
             entry_SYSCALL_64_after_hwframe+0x63/0xcd
    
                   other info that might help us debug this:
    
       Possible unsafe locking scenario:
    
             CPU0                    CPU1
             ----                    ----
        lock(cpu_hotplug_lock);
                                     lock(kvm_lock);
                                     lock(cpu_hotplug_lock);
        lock(kvm_lock);
    
                    *** DEADLOCK ***
    
      1 lock held by stable/251833:
       #0: ffffffffa2456828 (cpu_hotplug_lock){++++}-{0:0}, at: hardware_enable_all+0xf/0xc0 [kvm]
    Signed-off-by: default avatarSean Christopherson <seanjc@google.com>
    Message-Id: <20221130230934.1014142-16-seanjc@google.com>
    Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
    3af4a9e6
locking.rst 13.2 KB