Re: [RFC PATCH v2 04/10] kvm: kvm_luo: Allow kvm preservation with LUO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] KVM live update silently drops the VM type on non-x86 architectures, leading to incorrect VM initialization during restore.
- [High] Double free attempt of KHO serialized data on the successful retrieve path triggers a kernel warning.
- [Medium] Debugfs directory name collision for restored VMs silently drops statistics for new VMs.
--

commit ded3fe4d8e96caf44b395f665f8fedc9e63e601b
Author: Tarun Sahu <tarunsahu@google.com>

kvm: kvm_luo: Allow kvm preservation with LUO

Introduce KVM VM preservation support for Live Update Orchestrator.
This registers an LUO file handler for KVM files to serialize and
deserialize necessary VM state across live updates.

> diff --git a/virt/kvm/kvm_luo.c b/virt/kvm/kvm_luo.c
> new file mode 100644
> index 0000000000000..25619f94ace5b
> --- /dev/null
> +++ b/virt/kvm/kvm_luo.c
[ ... ]
> +static int kvm_luo_preserve(struct liveupdate_file_op_args *args)
> +{
[ ... ]
> +#ifdef CONFIG_X86
> +	ser->type = kvm->arch.vm_type;
> +#else
> +	ser->type = 0;

[Severity: High]
Does hardcoding the VM type to 0 on non-x86 architectures discard required
state? 

For instance, on ARM64, the VM type parameter encodes architectural
information such as the Intermediate Physical Address (IPA) size and
protection flags. 

Will restoring the VM with type 0 cause the new kernel to initialize the
VM with default settings, potentially leading to a mismatched memory layout?

> +#endif
> +
> +	args->serialized_data = virt_to_phys(ser);
> +
> +	return 0;
> +}
> +
> +static atomic_t restored_vm_id = ATOMIC_INIT(0);
> +
> +static int kvm_luo_retrieve(struct liveupdate_file_op_args *args)
> +{
> +	char fdname[ITOA_MAX_LEN + 1];
[ ... ]
> +	snprintf(fdname, sizeof(fdname), "%d",
> +		 atomic_inc_return(&restored_vm_id));

[Severity: Medium]
Since standard VMs use their native file descriptor numbers as fdname, and
those typically start from low numbers, will this auto-incrementing counter
guarantee namespace collisions? 

A collision here would result in a duplicate directory error in
kvm_create_vm_debugfs(), which would cause debugfs statistics for newly
created VMs to be skipped.

> +
> +	file = kvm_create_vm_file(ser->type, fdname);
[ ... ]
> +static void kvm_luo_finish(struct liveupdate_file_op_args *args)
> +{
> +	struct kvm_luo_ser *ser;
> +
> +	/*
> +	 * If retrieve_status is true or set to error, nothing to do here.
> +	 * Already cleaned up in kvm_luo_retrieve().
> +	 */
> +	if (args->retrieve_status)
> +		return;
> +
> +	if (!args->serialized_data)
> +		return;
> +
> +	ser = phys_to_virt(args->serialized_data);
> +	kho_restore_free(ser);

[Severity: High]
Can this result in a double-free on the successful retrieve path?

In kvm_luo_retrieve(), kho_restore_free() is explicitly called on success
before returning 0:

	args->file = file;
	kho_restore_free(ser);

	kvm_uevent_notify_vm_create(kvm);
	return 0;

Since the retrieve is successful, args->retrieve_status will be 0 here,
and this function will call kho_restore_free() again on the same
serialized data.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780676742.git.tarunsahu@google.com?part=4