| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a0090bbf-08b4-4b36-8cf2-18687a83ee8f@linux.microsoft.com>
Date: Fri, 17 Oct 2025 10:25:39 -0700
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.com>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc: "kys@...rosoft.com" <kys@...rosoft.com>,
"haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
"wei.liu@...nel.org" <wei.liu@...nel.org>,
"decui@...rosoft.com" <decui@...rosoft.com>, "arnd@...db.de"
<arnd@...db.de>, "mrathor@...ux.microsoft.com"
<mrathor@...ux.microsoft.com>,
"skinsburskii@...ux.microsoft.com" <skinsburskii@...ux.microsoft.com>
Subject: Re: [PATCH] mshv: Fix deposit memory in MSHV_ROOT_HVCALL
On 10/16/2025 6:12 PM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Thursday, October 16, 2025 12:54 PM
>>
>> When the MSHV_ROOT_HVCALL ioctl is executing a hypercall, and gets
>> HV_STATUS_INSUFFICIENT_MEMORY, it deposits memory and then returns
>> -EAGAIN to userspace.
>>
>> However, it's much easier and efficient if the driver simply deposits
>> memory on demand and immediately retries the hypercall as is done with
>> all the other hypercall helper functions.
>>
>> But unlike those, in MSHV_ROOT_HVCALL the input is opaque to the
>> kernel. This is problematic for rep hypercalls, because the next part
>> of the input list can't be copied on each loop after depositing pages
>> (this was the original reason for returning -EAGAIN in this case).
>>
>> Introduce hv_do_rep_hypercall_ex(), which adds a 'rep_start'
>> parameter. This solves the issue, allowing the deposit loop in
>> MSHV_ROOT_HVCALL to restart a rep hypercall after depositing pages
>> partway through.
>
>>>From reading the above, I'm pretty sure this code change is an
> optimization that lets user space avoid having to deal with the
> -EAGAIN result by resubmitting the ioctl with a different
> starting point for a rep hypercall. As such, I'd suggest the patch
> title should be "Improve deposit memory ...." (or something similar).
> The word "Fix" makes it sound like a bug fix.
>
> Or is user space code currently faulty in its handling of -EAGAIN, and
> this really is an indirect bug fix to make things work? If so, do you
> want a Fixes: tag so the change is backported?
>
It's the latter case, userspace doesn't handle it correctly, so I
consider it a fix more than just an improvement.
I'll add a Fixes: tag pointing back to the original /dev/mshv patch.
>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
>> ---
>> drivers/hv/mshv_root_main.c | 52 ++++++++++++++++++++--------------
>> include/asm-generic/mshyperv.h | 14 +++++++--
>> 2 files changed, 42 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
>> index 9ae67c6e9f60..731ec8cbbd63 100644
>> --- a/drivers/hv/mshv_root_main.c
>> +++ b/drivers/hv/mshv_root_main.c
>> @@ -159,6 +159,7 @@ static int mshv_ioctl_passthru_hvcall(struct mshv_partition *partition,
>> unsigned int pages_order;
>> void *input_pg = NULL;
>> void *output_pg = NULL;
>> + u16 reps_completed;
>>
>> if (copy_from_user(&args, user_args, sizeof(args)))
>> return -EFAULT;
>> @@ -210,28 +211,35 @@ static int mshv_ioctl_passthru_hvcall(struct mshv_partition *partition,
>> */
>> *(u64 *)input_pg = partition->pt_id;
>>
>> - if (args.reps)
>> - status = hv_do_rep_hypercall(args.code, args.reps, 0,
>> - input_pg, output_pg);
>> - else
>> - status = hv_do_hypercall(args.code, input_pg, output_pg);
>> -
>> - if (hv_result(status) == HV_STATUS_CALL_PENDING) {
>> - if (is_async) {
>> - mshv_async_hvcall_handler(partition, &status);
>> - } else { /* Paranoia check. This shouldn't happen! */
>> - ret = -EBADFD;
>> - goto free_pages_out;
>> + reps_completed = 0;
>> + do {
>> + if (args.reps) {
>> + status = hv_do_rep_hypercall_ex(args.code, args.reps,
>> + 0, reps_completed,
>> + input_pg, output_pg);
>> + reps_completed = hv_repcomp(status);
>> + } else {
>> + status = hv_do_hypercall(args.code, input_pg, output_pg);
>> }
>> - }
>>
>> - if (hv_result(status) == HV_STATUS_INSUFFICIENT_MEMORY) {
>> - ret = hv_call_deposit_pages(NUMA_NO_NODE, partition->pt_id, 1);
>> - if (!ret)
>> - ret = -EAGAIN;
>> - } else if (!hv_result_success(status)) {
>> - ret = hv_result_to_errno(status);
>> - }
>> + if (hv_result(status) == HV_STATUS_CALL_PENDING) {
>> + if (is_async) {
>> + mshv_async_hvcall_handler(partition, &status);
>> + } else { /* Paranoia check. This shouldn't happen! */
>> + ret = -EBADFD;
>> + goto free_pages_out;
>> + }
>> + }
>> +
>> + if (hv_result_success(status))
>> + break;
>> +
>> + if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY)
>> + ret = hv_result_to_errno(status);
>> + else
>> + ret = hv_call_deposit_pages(NUMA_NO_NODE,
>> + partition->pt_id, 1);
>> + } while (!ret);
>>
>> /*
>> * Always return the status and output data regardless of result.
>
> This comment about always returning the output data is now incorrect.
>
Thanks, I'll fix it
>> @@ -240,11 +248,11 @@ static int mshv_ioctl_passthru_hvcall(struct mshv_partition *partition,
>> * succeeded.
>> */
>> args.status = hv_result(status);
>> - args.reps = args.reps ? hv_repcomp(status) : 0;
>> + args.reps = reps_completed;
>> if (copy_to_user(user_args, &args, sizeof(args)))
>> ret = -EFAULT;
>>
>> - if (output_pg &&
>> + if (!ret && output_pg &&
>> copy_to_user((void __user *)args.out_ptr, output_pg, args.out_sz))
>> ret = -EFAULT;
>>
>> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
>> index ebf458dbcf84..31a209f0e18f 100644
>> --- a/include/asm-generic/mshyperv.h
>> +++ b/include/asm-generic/mshyperv.h
>> @@ -128,8 +128,9 @@ static inline unsigned int hv_repcomp(u64 status)
>> * Rep hypercalls. Callers of this functions are supposed to ensure that
>> * rep_count and varhead_size comply with Hyper-V hypercall definition.
>
> Nit: This comment could be updated to include the new "rep_start"
> parameter.
>
Thanks, will add
>> */
>> -static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>> - void *input, void *output)
>> +static inline u64 hv_do_rep_hypercall_ex(u16 code, u16 rep_count,
>> + u16 varhead_size, u16 rep_start,
>> + void *input, void *output)
>> {
>> u64 control = code;
>> u64 status;
>> @@ -137,6 +138,7 @@ static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>>
>> control |= (u64)varhead_size << HV_HYPERCALL_VARHEAD_OFFSET;
>> control |= (u64)rep_count << HV_HYPERCALL_REP_COMP_OFFSET;
>> + control |= (u64)rep_start << HV_HYPERCALL_REP_START_OFFSET;
>>
>> do {
>> status = hv_do_hypercall(control, input, output);
>> @@ -154,6 +156,14 @@ static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>> return status;
>> }
>>
>> +/* For the typical case where rep_start is 0 */
>> +static inline u64 hv_do_rep_hypercall(u16 code, u16 rep_count, u16 varhead_size,
>> + void *input, void *output)
>> +{
>> + return hv_do_rep_hypercall_ex(code, rep_count, varhead_size, 0,
>> + input, output);
>> +}
>> +
>> /* Generate the guest OS identifier as described in the Hyper-V TLFS */
>> static inline u64 hv_generate_guest_id(u64 kernel_version)
>> {
>
> Overall, this looks good to me. I don't see any issues with the code.
>
> Michael
Powered by blists - more mailing lists