Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] KVM: debugfs dentry removed at VM fd release while vcpu/device fd paths still use it - [Low] kvm: destroy uevent loses STATS_PATH once VM fd is released - [Low] KVM: debugfs dir removed at file release, not when the fd number is freed --
commit beaae50d646de519b7a984e475c82532587d2d5a Author: Andrey Drobyshev <[email protected]> KVM: remove the VM's debugfs directory when its fd is released This patch removes the per-VM debugfs directory in kvm_vm_release(), when the VM fd is released, instead of only in kvm_destroy_vm(). The stat data is still freed at VM destruction. This is meant to avoid "duplicate directory" collisions when a VM outlives its fd and the fd number is reused. Link: https://virtuozzo.atlassian.net/browse/VSTOR-150597 > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c [ ... ] > +static void kvm_remove_vm_debugfs(struct kvm *kvm) > +{ > + if (IS_ERR(kvm->debugfs_dentry)) > + return; > + > + debugfs_remove_recursive(kvm->debugfs_dentry); > + kvm->debugfs_dentry = ERR_PTR(-ENOENT); > +} [Severity: High] Can this race with readers of kvm->debugfs_dentry that hold only a kvm reference and not the VM fd? Before this patch the dentry was removed only in kvm_destroy_vm(), when users_count had reached zero, so any holder of a kvm reference could use it. Now the dentry is removed while struct kvm stays live through vcpu fds, device fds or VFIO. kvm_remove_vm_debugfs() removes the directory and then stores ERR_PTR(-ENOENT) with a plain store, with no lock and no READ_ONCE()/WRITE_ONCE(). Consider these two paths on arm64. vcpu fd path, on first KVM_RUN: vgic_lazy_init() vgic_init() vgic_debug_init(kvm) debugfs_create_file("vgic-state", 0444, kvm->debugfs_dentry, kvm, &vgic_debug_fops); Only kvm->arch.config_lock is held here, and kvm_vm_release() never takes it. device fd path: KVM_SET_DEVICE_ATTR (KVM_VGIC_ITS_ADDR_TYPE) on an ITS device fd vgic_its_debug_init() debugfs_create_file(name, 0444, dev->kvm->debugfs_dentry, its, &vgic_its_debug_fops); A device fd pins struct kvm but not the VM fd. The same applies to KVM_DEV_ARM_VGIC_GRP_CTRL reaching vgic_init() and then vgic_debug_init(). If another thread closes the last VM fd, a reader that loaded the old pointer before the store reaches start_creating() in fs/debugfs/inode.c: start_creating() if (IS_ERR(parent)) return parent; ... inode_lock(d_inode(parent)); if (unlikely(IS_DEADDIR(d_inode(parent)))) The IS_ERR(parent) check only helps a reader that sees the new value. Otherwise start_creating() locks and inspects a dentry and inode that may already be freed, or creates a file under a directory that is being removed. Can this cause a use-after-free of the debugfs parent dentry and inode? The window is narrow, but it matches the cpr-exec/VFIO scenario this patch targets. Should these users be serialized against kvm_vm_release()? [ ... ] > @@ -1364,6 +1371,9 @@ static int kvm_vm_release(struct inode *inode, struct > file *filp) > > kvm_irqfd_release(kvm); > > + /* The directory is named after this fd, which is free to reuse now */ > + kvm_remove_vm_debugfs(kvm); > + [Severity: Low] Is the comment accurate? kvm_vm_release() runs at the last fput() of the struct file, not when the fd number is freed. close_fd() frees the fd slot under file_lock first: close_fd() spin_lock(&files->file_lock); file = file_close_fd_locked(files, fd); spin_unlock(&files->file_lock); ... return filp_close(file, files); The final fput() then runs later as task work. In a multithreaded process, another thread can get the same fd number and call KVM_CREATE_VM before the deferred release has run. If the file has other holders (dup(), SCM_RIGHTS, another process), the fd number is reusable while the old directory still exists. In both cases kvm_create_vm_debugfs() still hits this path: snprintf(dir_name, sizeof(dir_name), "%d-%s", task_pid_nr(current), fdname); mutex_lock(&kvm_debugfs_lock); dent = debugfs_lookup(dir_name, kvm_debugfs_dir); if (dent) { pr_warn_ratelimited("KVM: debugfs: duplicate directory %s\n", dir_name); The new VM then runs without debugfs stats, as in the original problem. Does the claim that the name is free to reuse once the fd is released hold only for the simple single-threaded close-then-create case? > kvm_put_kvm(kvm); > return 0; > } [Severity: Low] This isn't a crash, but the destroy uevent no longer carries STATS_PATH in the usual case. kvm_remove_vm_debugfs() sets kvm->debugfs_dentry to ERR_PTR(-ENOENT) before the final kvm_put_kvm(). The final kvm_put_kvm() then reaches kvm_destroy_vm(), which calls: kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm); kvm_destroy_vm_debugfs(kvm); In kvm_uevent_notify_change() the STATS_PATH block is guarded by if (!IS_ERR(kvm->debugfs_dentry)), so it is skipped. This is not limited to the VFIO and cpr-exec cases. Whenever the VM fd is closed before the last kvm reference is dropped, the destroy uevent omits STATS_PATH. Before the patch it was always present. The commit message mentions this change, but is the userspace-visible change to the destroy uevent intended for all VMs? -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=2 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
