linux-kernel - Re: [PATCH 1/3] mshv: Ignore second stats page map result failure

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ae9b13d-b809-431a-b71f-31014b23ae0d@linux.microsoft.com>
Date: Wed, 3 Dec 2025 13:43:35 -0800
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
Cc: linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
 kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
 decui@...rosoft.com, longli@...rosoft.com, mhklinux@...look.com,
 prapal@...ux.microsoft.com, mrathor@...ux.microsoft.com,
 paekkaladevi@...ux.microsoft.com
Subject: Re: [PATCH 1/3] mshv: Ignore second stats page map result failure

On 12/3/2025 11:03 AM, Stanislav Kinsburskii wrote:
> On Wed, Dec 03, 2025 at 09:53:23AM -0800, Nuno Das Neves wrote:
>> 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].
>>
>> Signed-off-by: Purna Pavan Chandra Aekkaladevi <paekkaladevi@...ux.microsoft.com>
>> Signed-off-by: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
>> ---
>>  drivers/hv/mshv_root_hv_call.c | 43 ++++++++++++++++++++++++++++++----
>>  drivers/hv/mshv_root_main.c    |  3 +++
>>  2 files changed, 42 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
>> index 598eaff4ff29..0427785bb7fe 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,28 @@ 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) {
>> +				pr_debug_once("%s: PARENT area type is unsupported\n",
>> +					      __func__);
> 
> Nit: this gebug once looks a bit odd. Why not having it printed always
> (especially given the unconfidional status print a few lines below)?
> 

This isn't indicative of an error, but a missing feature in an older
hypervisor. The rationale of _once is that this information only needs
to be printed once.

Now that I think about it however, it doesn't really change how the
statistics would be interpreted. In the situation where this condition
is triggered (root partition on old hypervisor), all the stats info
is in the HV_STATS_AREA_SELF page anyway. So, maybe this can just be
removed.

> Reviewed-by: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
> 
> Thanks,
> Stanislav
> 
>> +				*addr = NULL;
>> +				return 0;
>> +			}
>> +
>> +			hv_status_debug(status, "\n");
>> +			return hv_result_to_errno(status);
>>  		}
>>  
>>  		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
>>