| 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:
<SN6PR02MB4157E6D02773A9D7A4B9E85BD4F6A@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Fri, 17 Oct 2025 01:12:57 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Nuno Das Neves <nunodasneves@...ux.microsoft.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
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?
>
> 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.
> @@ -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.
> */
> -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