linux-kernel - RE: [PATCH] Drivers: hv: vmbus: Remove the per-CPU post_msg

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]

Date:   Tue, 11 Apr 2023 17:24:31 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Dexuan Cui <decui@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] Drivers: hv: vmbus: Remove the per-CPU post_msg_page

From: Dexuan Cui <decui@...rosoft.com> Sent: Saturday, April 8, 2023 2:35 PM
> 
> The post_msg_page was introduced in 2014 in
> commit b29ef3546aec ("Drivers: hv: vmbus: Cleanup hv_post_message()")
> 
> Commit 68bb7bfb7985 ("X86/Hyper-V: Enable IPI enlightenments") introduced
> the hyperv_pcpu_input_arg in 2018, which can be used in hv_post_message().
> 
> Remove post_msg_page to simplify the code a little bit.
> 
> Signed-off-by: Dexuan Cui <decui@...rosoft.com>
> ---
>  drivers/hv/hv.c           | 20 +++++---------------
>  drivers/hv/hyperv_vmbus.h |  4 ----
>  2 files changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 8b0dd8e5244d7..30bf122a502ae 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -84,14 +84,15 @@ int hv_post_message(union hv_connection_id connection_id,
>  		  void *payload, size_t payload_size)
>  {
>  	struct hv_input_post_message *aligned_msg;
> -	struct hv_per_cpu_context *hv_cpu;
> +	unsigned long flags;
>  	u64 status;
> 
>  	if (payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT)
>  		return -EMSGSIZE;
> 
> -	hv_cpu = get_cpu_ptr(hv_context.cpu_context);
> -	aligned_msg = hv_cpu->post_msg_page;
> +	local_irq_save(flags);
> +
> +	aligned_msg = *this_cpu_ptr(hyperv_pcpu_input_arg);
>  	aligned_msg->connectionid = connection_id;
>  	aligned_msg->reserved = 0;
>  	aligned_msg->message_type = message_type;
> @@ -106,11 +107,7 @@ int hv_post_message(union hv_connection_id connection_id,
>  		status = hv_do_hypercall(HVCALL_POST_MESSAGE,
>  				aligned_msg, NULL);
> 
> -	/* Preemption must remain disabled until after the hypercall
> -	 * so some other thread can't get scheduled onto this cpu and
> -	 * corrupt the per-cpu post_msg_page
> -	 */
> -	put_cpu_ptr(hv_cpu);
> +	local_irq_restore(flags);
> 
>  	return hv_result(status);
>  }
> @@ -162,12 +159,6 @@ int hv_synic_alloc(void)
>  				goto err;
>  			}
>  		}
> -
> -		hv_cpu->post_msg_page = (void *)get_zeroed_page(GFP_ATOMIC);
> -		if (hv_cpu->post_msg_page == NULL) {
> -			pr_err("Unable to allocate post msg page\n");
> -			goto err;
> -		}
>  	}
> 
>  	return 0;
> @@ -190,7 +181,6 @@ void hv_synic_free(void)
> 
>  		free_page((unsigned long)hv_cpu->synic_event_page);
>  		free_page((unsigned long)hv_cpu->synic_message_page);
> -		free_page((unsigned long)hv_cpu->post_msg_page);
>  	}
> 
>  	kfree(hv_context.hv_numa_map);
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index dc673edf053c3..d8322049ecd08 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -122,10 +122,6 @@ enum {
>  struct hv_per_cpu_context {
>  	void *synic_message_page;
>  	void *synic_event_page;
> -	/*
> -	 * buffer to post messages to the host.
> -	 */
> -	void *post_msg_page;
> 
>  	/*
>  	 * Starting with win8, we can take channel interrupts on any CPU;
> --
> 2.25.1

This looks good.  Glad to have this simplification done!

FWIW, this patch will cause conflicts with your TDX patch series.  If this
patch goes first, then you'll have merge errors with the TDX patches.
If the TDX series goes first, then this patch will have merge errors.  And
per my comments on Patch 5 of your TDX series, eliminating the
post_msg_page will simplify the error cleanup code in hv_synic_alloc().
It seems like making this patch part of the TDX series would be helpful
all around.

Reviewed-by: Michael Kelley