| 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:
<SN6PR02MB41578C85BD5C114340677F84D4A2A@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 8 Dec 2025 15:12:58 +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>,
"skinsburskii@...ux.microsoft.com" <skinsburskii@...ux.microsoft.com>
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>, "longli@...rosoft.com"
<longli@...rosoft.com>, "prapal@...ux.microsoft.com"
<prapal@...ux.microsoft.com>, "mrathor@...ux.microsoft.com"
<mrathor@...ux.microsoft.com>, "paekkaladevi@...ux.microsoft.com"
<paekkaladevi@...ux.microsoft.com>
Subject: RE: [PATCH v2 1/3] mshv: Ignore second stats page map result failure
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Friday, December 5, 2025 10:59 AM
>
> From: Purna Pavan Chandra Aekkaladevi <paekkaladevi@...ux.microsoft.com>
>
> Older versions of the hypervisor do not support HV_STATS_AREA_PARENT
> and return HV_STATUS_INVALID_PARAMETER for the second stats page
> mapping request.
>
> This results a failure in module init. Instead of failing, gracefully
> fall back to populating stats_pages[HV_STATS_AREA_PARENT] with the
> already-mapped stats_pages[HV_STATS_AREA_SELF].
This explains "what" this patch does. But could you add an explanation of "why"
substituting SELF for the unavailable PARENT is the right thing to do? As a somewhat
outside reviewer, I don't know enough about SELF vs. PARENT to immediately know
why this substitution makes sense.
Also, does this patch affect the logic in mshv_vp_dispatch_thread_blocked() where
a zero value for the SELF version of VpRootDispatchThreadBlocked is replaced by
the PARENT value? But that logic seems to be in the reverse direction -- replacing
a missing SELF value with the PARENT value -- whereas this patch is about replacing
missing PARENT values with SELF values. So are there two separate PARENT vs. SELF
issues overall? And after this patch is in place and PARENT values are replaced with
SELF on older hypervisor versions, the logic in mshv_vp_dispatch_thread_blocked()
then effectively becomes a no-op if the SELF value is zero, and the return value will
be zero. Is that problem?
>
> Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@...ux.microsoft.com>
> Signed-off-by: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
> Reviewed-by: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
> ---
> drivers/hv/mshv_root_hv_call.c | 41 ++++++++++++++++++++++++++++++----
> drivers/hv/mshv_root_main.c | 3 +++
> 2 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
> index 598eaff4ff29..b1770c7b500c 100644
> --- a/drivers/hv/mshv_root_hv_call.c
> +++ b/drivers/hv/mshv_root_hv_call.c
> @@ -855,6 +855,24 @@ static int hv_call_map_stats_page2(enum
> hv_stats_object_type type,
> return ret;
> }
>
> +static int
> +hv_stats_get_area_type(enum hv_stats_object_type type,
> + const union hv_stats_object_identity *identity)
> +{
> + switch (type) {
> + case HV_STATS_OBJECT_HYPERVISOR:
> + return identity->hv.stats_area_type;
> + case HV_STATS_OBJECT_LOGICAL_PROCESSOR:
> + return identity->lp.stats_area_type;
> + case HV_STATS_OBJECT_PARTITION:
> + return identity->partition.stats_area_type;
> + case HV_STATS_OBJECT_VP:
> + return identity->vp.stats_area_type;
> + }
> +
> + return -EINVAL;
> +}
> +
> static int hv_call_map_stats_page(enum hv_stats_object_type type,
> const union hv_stats_object_identity *identity,
> void **addr)
> @@ -863,7 +881,7 @@ static int hv_call_map_stats_page(enum hv_stats_object_type type,
> struct hv_input_map_stats_page *input;
> struct hv_output_map_stats_page *output;
> u64 status, pfn;
> - int ret = 0;
> + int hv_status, ret = 0;
>
> do {
> local_irq_save(flags);
> @@ -878,11 +896,26 @@ static int hv_call_map_stats_page(enum hv_stats_object_type type,
> pfn = output->map_location;
>
> local_irq_restore(flags);
> - if (hv_result(status) != HV_STATUS_INSUFFICIENT_MEMORY) {
> - ret = hv_result_to_errno(status);
> +
> + hv_status = hv_result(status);
> + if (hv_status != HV_STATUS_INSUFFICIENT_MEMORY) {
> if (hv_result_success(status))
> break;
> - return ret;
> +
> + /*
> + * Older versions of the hypervisor do not support the
> + * PARENT stats area. In this case return "success" but
> + * set the page to NULL. The caller should check for
> + * this case and instead just use the SELF area.
> + */
> + if (hv_stats_get_area_type(type, identity) == HV_STATS_AREA_PARENT &&
> + hv_status == HV_STATUS_INVALID_PARAMETER) {
> + *addr = NULL;
> + return 0;
> + }
> +
> + hv_status_debug(status, "\n");
> + return hv_result_to_errno(status);
Does the hv_call_map_stats_page2() function need a similar fix? Or is there a linkage
in hypervisor functionality where any hypervisor version that supports an overlay GPFN
also supports the PARENT stats? If such a linkage is why hv_call_map_stats_page2()
doesn't need a similar fix, please add a code comment to that effect in
hv_call_map_stats_page2().
> }
>
> ret = hv_call_deposit_pages(NUMA_NO_NODE,
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index bc15d6f6922f..f59a4ab47685 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -905,6 +905,9 @@ static int mshv_vp_stats_map(u64 partition_id, u32 vp_index,
> if (err)
> goto unmap_self;
>
> + if (!stats_pages[HV_STATS_AREA_PARENT])
> + stats_pages[HV_STATS_AREA_PARENT] =
> stats_pages[HV_STATS_AREA_SELF];
> +
> return 0;
>
> unmap_self:
> --
> 2.34.1
Powered by blists - more mailing lists