On Wed, Jan 24, 2024 at 11:04:55AM -0800, Sean Christopherson wrote: > On Sat, Jan 20, 2024, Xu Yilun wrote: > > On Tue, Jan 09, 2024 at 05:15:30PM -0800, Sean Christopherson wrote: > > > Always flush the per-vCPU async #PF workqueue when a vCPU is clearing its > > > completion queue, e.g. when a VM and all its vCPUs is being destroyed. > > > KVM must ensure that none of its workqueue callbacks is running when the > > > last reference to the KVM _module_ is put. Gifting a reference to the > > > associated VM prevents the workqueue callback from dereferencing freed > > > vCPU/VM memory, but does not prevent the KVM module from being unloaded > > > before the callback completes. > > > > > > Drop the misguided VM refcount gifting, as calling kvm_put_kvm() from > > > async_pf_execute() if kvm_put_kvm() flushes the async #PF workqueue will > > > result in deadlock. async_pf_execute() can't return until kvm_put_kvm() > > > finishes, and kvm_put_kvm() can't return until async_pf_execute() finishes: > > > > > > WARNING: CPU: 8 PID: 251 at virt/kvm/kvm_main.c:1435 kvm_put_kvm+0x2d/0x320 [kvm] > > > Modules linked in: vhost_net vhost vhost_iotlb tap kvm_intel kvm irqbypass > > > CPU: 8 PID: 251 Comm: kworker/8:1 Tainted: G W 6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119 > > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 > > > Workqueue: events async_pf_execute [kvm] > > > RIP: 0010:kvm_put_kvm+0x2d/0x320 [kvm] > > > Call Trace: > > > <TASK> > > > async_pf_execute+0x198/0x260 [kvm] > > > process_one_work+0x145/0x2d0 > > > worker_thread+0x27e/0x3a0 > > > kthread+0xba/0xe0 > > > ret_from_fork+0x2d/0x50 > > > ret_from_fork_asm+0x11/0x20 > > > </TASK> > > > ---[ end trace 0000000000000000 ]--- > > > INFO: task kworker/8:1:251 blocked for more than 120 seconds. > > > Tainted: G W 6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119 > > > "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > > > task:kworker/8:1 state:D stack:0 pid:251 ppid:2 flags:0x00004000 > > > Workqueue: events async_pf_execute [kvm] > > > Call Trace: > > > <TASK> > > > __schedule+0x33f/0xa40 > > > schedule+0x53/0xc0 > > > schedule_timeout+0x12a/0x140 > > > __wait_for_common+0x8d/0x1d0 > > > __flush_work.isra.0+0x19f/0x2c0 > > > kvm_clear_async_pf_completion_queue+0x129/0x190 [kvm] > > > kvm_arch_destroy_vm+0x78/0x1b0 [kvm] > > > kvm_put_kvm+0x1c1/0x320 [kvm] > > > async_pf_execute+0x198/0x260 [kvm] > > > process_one_work+0x145/0x2d0 > > > worker_thread+0x27e/0x3a0 > > > kthread+0xba/0xe0 > > > ret_from_fork+0x2d/0x50 > > > ret_from_fork_asm+0x11/0x20 > > > </TASK> > > > > > > If kvm_clear_async_pf_completion_queue() actually flushes the workqueue, > > > then there's no need to gift async_pf_execute() a reference because all > > > invocations of async_pf_execute() will be forced to complete before the > > > vCPU and its VM are destroyed/freed. And that in turn fixes the module > > > unloading bug as __fput() won't do module_put() on the last vCPU reference > > > until the vCPU has been freed, e.g. if closing the vCPU file also puts the > > > > I'm not sure why __fput() of vCPU fd should be mentioned here. I assume > > we just need to say that vCPUs are freed before module_put(KVM the module) > > in kvm_destroy_vm(), then the whole logic for module unloading fix is: > > > > 1. All workqueue callbacks complete when kvm_clear_async_pf_completion_queue(vcpu) > > 2. kvm_clear_async_pf_completion_queue(vcpu) must be executed before vCPU free. > > 3. vCPUs must be freed before module_put(KVM the module). > > > > So all workqueue callbacks complete before module_put(KVM the module). > > > > > > __fput() of vCPU fd is not the only trigger of kvm_destroy_vm(), that > > makes me distracted from reason of the fix. > > My goal was to call out that (a) the vCPU file descriptor is what ensures kvm.ko > is alive at this point and (b) that __fput() very deliberately ensures module_put() > is called after all module function callbacks/hooks complete, as there was quite Ah, I understood. These are ensured by your previous fix which grants kvm_vcpu_fops the module owner. LGTM now. Thanks, Yilun
