Re: [Devel] [PATCH hci-8.1 2/2] KVM: remove the VM's debugfs directory when its fd is released

[no-reply@sashiko.virtuozzo.com]

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 <andrey.drobyshev@virtuozzo.com>

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/20261105121733.643754-1-andrey.drobyshev@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel