Re: [PATCH v1 03/13] mshv: add SEV-SNP isolated page hypercalls

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `completed` variable is evaluated from `status` before the asynchronous completion handler updates it, leading to a stale completion count and an infinite loop.
--

--- Patch [3]: [PATCH v1 03/13] mshv: add SEV-SNP isolated page hypercalls ---
commit 3c19c21fc983589586a3386b97c714ce13aec72c
Author: Wei Liu <[email protected]>

mshv: add SEV-SNP isolated page hypercalls

Add helpers for importing isolated pages and completing isolated
imports.  These are used by SEV-SNP guest launch flows to transfer
isolated page state to the hypervisor.

> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
> index 48abe338c06f8..9ddc8dd73da7a 100644
> --- a/drivers/hv/mshv_root_hv_call.c
> +++ b/drivers/hv/mshv_root_hv_call.c
[ ... ]
> @@ -1010,6 +1014,99 @@ int hv_unmap_stats_page(enum hv_stats_object_type type,
>  }
>  
>  #ifdef HV_SUPPORTS_SEV_SNP_GUESTS
> +int hv_call_import_isolated_pages(u64 partition_id, u64 *pages,
> +				  u64 num_pages,
> +				  enum hv_isolated_page_type page_type,
> +				  enum hv_isolated_page_size page_size,
> +				  void (*completion_handler)(void *data,
> +							     u64 *status),
> +				  void *completion_data)
> +{
[ ... ]
> +	while (remaining) {
> +		rep_count = min_t(unsigned long, remaining,
> +				  HV_ISOLATED_PAGE_BATCH_SIZE);
> +
> +		local_irq_save(flags);
> +		input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +		memset(input, 0, sizeof(*input));
> +		input->partition_id = partition_id;
> +		input->page_type = page_type;
> +		input->page_size = page_size;
> +		memcpy(input->page_number, gpa, rep_count * sizeof(*gpa));
> +		status = hv_do_rep_hypercall(HVCALL_IMPORT_ISOLATED_PAGES,
> +					     rep_count, 0, input, NULL);
> +		local_irq_restore(flags);
> +
> +		completed = hv_repcomp(status);
> +		if (hv_result(status) == HV_STATUS_CALL_PENDING)
> +			completion_handler(completion_data, &status);

[Severity: High]
If the hypercall returns HV_STATUS_CALL_PENDING, the completion_handler updates
the status variable, but the completed variable was already evaluated from the
initial status.

Could this lead to completed being 0 (or a stale value), which would then
prevent gpa and remaining from advancing correctly?

> +
> +		if (!hv_result_success(status)) {
> +			pr_err("%s: completed %llu of %llu, %s\n", __func__,
> +			       num_pages - remaining, num_pages,
> +			       hv_result_to_string(status));
> +			return hv_result_to_errno(status);
> +		}
> +
> +		gpa += completed;
> +		remaining -= completed;

[Severity: High]
If completed is not updated after the completion handler executes, does this
result in an infinite loop since remaining will not decrease?

> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3