* Elliot Berman <quic_eberman@xxxxxxxxxxx> [2023-02-14 13:24:26]:
> static void gh_vm_free(struct work_struct *work)
> {
> struct gh_vm *ghvm = container_of(work, struct gh_vm, free_work);
> struct gh_vm_mem *mapping, *tmp;
> int ret;
>
> - mutex_lock(&ghvm->mm_lock);
> - list_for_each_entry_safe(mapping, tmp, &ghvm->memory_mappings, list) {
> - gh_vm_mem_reclaim(ghvm, mapping);
> - kfree(mapping);
> + switch (ghvm->vm_status) {
> +unknown_state:
> + case GH_RM_VM_STATUS_RUNNING:
> + gh_vm_stop(ghvm);
> + fallthrough;
> + case GH_RM_VM_STATUS_INIT_FAILED:
> + case GH_RM_VM_STATUS_LOAD:
> + case GH_RM_VM_STATUS_LOAD_FAILED:
> + mutex_lock(&ghvm->mm_lock);
> + list_for_each_entry_safe(mapping, tmp, &ghvm->memory_mappings, list) {
> + gh_vm_mem_reclaim(ghvm, mapping);
> + kfree(mapping);
> + }
> + mutex_unlock(&ghvm->mm_lock);
> + fallthrough;
> + case GH_RM_VM_STATUS_NO_STATE:
> + ret = gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid);
> + if (ret)
> + pr_warn("Failed to deallocate vmid: %d\n", ret);
> +
> + gh_rm_notifier_unregister(ghvm->rm, &ghvm->nb);
> + put_gh_rm(ghvm->rm);
> + kfree(ghvm);
> + break;
> + default:
> + pr_err("VM is unknown state: %d, assuming it's running.\n", ghvm->vm_status);
> + goto unknown_state;
'goto unknown_state' here leads to a infinite loop AFAICS. For example consider
the case where VM_START failed (due to mem_lend operation) causing VM state to
be GH_RM_VM_STATUS_RESET. A subsequent close(vmfd) can leads to that forever
loop.
//snip
> +static int gh_vm_start(struct gh_vm *ghvm)
> +{
> + struct gh_vm_mem *mapping;
> + u64 dtb_offset;
> + u32 mem_handle;
> + int ret;
> +
> + down_write(&ghvm->status_lock);
> + if (ghvm->vm_status != GH_RM_VM_STATUS_LOAD) {
> + up_write(&ghvm->status_lock);
> + return 0;
> + }
> +
> + ghvm->vm_status = GH_RM_VM_STATUS_RESET;
> +
> + list_for_each_entry(mapping, &ghvm->memory_mappings, list) {
We don't seem to have the right lock here while walking the list.
