RE: [PATCH V5 04/12] Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM

[Michael Kelley <mikelley@microsoft.com>]

From: Tianyu Lan <ltykernel@gmail.com> Sent: Tuesday, September 14, 2021 6:39 AM
> 
> Mark vmbus ring buffer visible with set_memory_decrypted() when
> establish gpadl handle.
> 
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> ---
> Change sincv v4
> 	* Change gpadl handle in netvsc and uio driver from u32 to
> 	  struct vmbus_gpadl.
> 	* Change vmbus_establish_gpadl()'s gpadl_handle parameter
> 	  to vmbus_gpadl data structure.
> 
> Change since v3:
> 	* Change vmbus_teardown_gpadl() parameter and put gpadl handle,
> 	  buffer and buffer size in the struct vmbus_gpadl.
> ---
>  drivers/hv/channel.c            | 54 ++++++++++++++++++++++++---------
>  drivers/net/hyperv/hyperv_net.h |  5 +--
>  drivers/net/hyperv/netvsc.c     | 17 ++++++-----
>  drivers/uio/uio_hv_generic.c    | 20 ++++++------
>  include/linux/hyperv.h          | 12 ++++++--
>  5 files changed, 71 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index f3761c73b074..cf419eb1de77 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -17,6 +17,7 @@
>  #include <linux/hyperv.h>
>  #include <linux/uio.h>
>  #include <linux/interrupt.h>
> +#include <linux/set_memory.h>
>  #include <asm/page.h>
>  #include <asm/mshyperv.h>
> 
> @@ -456,7 +457,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  				   enum hv_gpadl_type type, void *kbuffer,
>  				   u32 size, u32 send_offset,
> -				   u32 *gpadl_handle)
> +				   struct vmbus_gpadl *gpadl)
>  {
>  	struct vmbus_channel_gpadl_header *gpadlmsg;
>  	struct vmbus_channel_gpadl_body *gpadl_body;
> @@ -474,6 +475,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  	if (ret)
>  		return ret;
> 
> +	ret = set_memory_decrypted((unsigned long)kbuffer,
> +				   HVPFN_UP(size));

This should be PFN_UP, not HVPFN_UP.  The numpages parameter to
set_memory_decrypted() is in guest size pages, not Hyper-V size pages.

> +	if (ret) {
> +		dev_warn(&channel->device_obj->device,
> +			 "Failed to set host visibility for new GPADL %d.\n",
> +			 ret);
> +		return ret;
> +	}
> +
>  	init_completion(&msginfo->waitevent);
>  	msginfo->waiting_channel = channel;
> 
> @@ -537,7 +547,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  	}
> 
>  	/* At this point, we received the gpadl created msg */
> -	*gpadl_handle = gpadlmsg->gpadl;
> +	gpadl->gpadl_handle = gpadlmsg->gpadl;
> +	gpadl->buffer = kbuffer;
> +	gpadl->size = size;
> +
> 
>  cleanup:
>  	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
> @@ -549,6 +562,11 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>  	}
> 
>  	kfree(msginfo);
> +
> +	if (ret)
> +		set_memory_encrypted((unsigned long)kbuffer,
> +				     HVPFN_UP(size));

Should be PFN_UP as noted on the previous call to set_memory_decrypted().

> +
>  	return ret;
>  }
> 
> @@ -561,10 +579,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>   * @gpadl_handle: some funky thing
>   */
>  int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
> -			  u32 size, u32 *gpadl_handle)
> +			  u32 size, struct vmbus_gpadl *gpadl)
>  {
>  	return __vmbus_establish_gpadl(channel, HV_GPADL_BUFFER, kbuffer, size,
> -				       0U, gpadl_handle);
> +				       0U, gpadl);
>  }
>  EXPORT_SYMBOL_GPL(vmbus_establish_gpadl);
> 
> @@ -639,6 +657,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  	struct vmbus_channel_open_channel *open_msg;
>  	struct vmbus_channel_msginfo *open_info = NULL;
>  	struct page *page = newchannel->ringbuffer_page;
> +	struct vmbus_gpadl gpadl;

I think this local variable was needed in a previous version of the patch, but
is now unused and should be deleted.

>  	u32 send_pages, recv_pages;
>  	unsigned long flags;
>  	int err;
> @@ -675,7 +694,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  		goto error_clean_ring;
> 
>  	/* Establish the gpadl for the ring buffer */
> -	newchannel->ringbuffer_gpadlhandle = 0;
> +	newchannel->ringbuffer_gpadlhandle.gpadl_handle = 0;
> 
>  	err = __vmbus_establish_gpadl(newchannel, HV_GPADL_RING,
>  				      page_address(newchannel->ringbuffer_page),
> @@ -701,7 +720,8 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  	open_msg->header.msgtype = CHANNELMSG_OPENCHANNEL;
>  	open_msg->openid = newchannel->offermsg.child_relid;
>  	open_msg->child_relid = newchannel->offermsg.child_relid;
> -	open_msg->ringbuffer_gpadlhandle = newchannel->ringbuffer_gpadlhandle;
> +	open_msg->ringbuffer_gpadlhandle
> +		= newchannel->ringbuffer_gpadlhandle.gpadl_handle;
>  	/*
>  	 * The unit of ->downstream_ringbuffer_pageoffset is HV_HYP_PAGE and
>  	 * the unit of ->ringbuffer_send_offset (i.e. send_pages) is PAGE, so
> @@ -759,8 +779,8 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  error_free_info:
>  	kfree(open_info);
>  error_free_gpadl:
> -	vmbus_teardown_gpadl(newchannel, newchannel->ringbuffer_gpadlhandle);
> -	newchannel->ringbuffer_gpadlhandle = 0;
> +	vmbus_teardown_gpadl(newchannel, &newchannel->ringbuffer_gpadlhandle);
> +	newchannel->ringbuffer_gpadlhandle.gpadl_handle = 0;

My previous comments had suggested letting vmbus_teardown_gpadl() set the
gpadl_handle to zero, avoiding the need for all the callers to set it to zero.
Did that not work for some reason?  Just curious ....

>  error_clean_ring:
>  	hv_ringbuffer_cleanup(&newchannel->outbound);
>  	hv_ringbuffer_cleanup(&newchannel->inbound);
> @@ -806,7 +826,7 @@ EXPORT_SYMBOL_GPL(vmbus_open);
>  /*
>   * vmbus_teardown_gpadl -Teardown the specified GPADL handle
>   */
> -int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
> +int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpadl)
>  {
>  	struct vmbus_channel_gpadl_teardown *msg;
>  	struct vmbus_channel_msginfo *info;
> @@ -825,7 +845,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
> 
>  	msg->header.msgtype = CHANNELMSG_GPADL_TEARDOWN;
>  	msg->child_relid = channel->offermsg.child_relid;
> -	msg->gpadl = gpadl_handle;
> +	msg->gpadl = gpadl->gpadl_handle;
> 
>  	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
>  	list_add_tail(&info->msglistentry,
> @@ -859,6 +879,12 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
> 
>  	kfree(info);
> +
> +	ret = set_memory_encrypted((unsigned long)gpadl->buffer,
> +				   HVPFN_UP(gpadl->size));

PFN_UP here as well.

> +	if (ret)
> +		pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> @@ -896,6 +922,7 @@ void vmbus_reset_channel_cb(struct vmbus_channel *channel)
>  static int vmbus_close_internal(struct vmbus_channel *channel)
>  {
>  	struct vmbus_channel_close_channel *msg;
> +	struct vmbus_gpadl gpadl;

I think this local variable was needed in a previous version of the patch, but
is now unused and should be deleted.

>  	int ret;
> 
>  	vmbus_reset_channel_cb(channel);
> @@ -933,9 +960,8 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
>  	}
> 
>  	/* Tear down the gpadl for the channel's ring buffer */
> -	else if (channel->ringbuffer_gpadlhandle) {
> -		ret = vmbus_teardown_gpadl(channel,
> -					   channel->ringbuffer_gpadlhandle);
> +	else if (channel->ringbuffer_gpadlhandle.gpadl_handle) {
> +		ret = vmbus_teardown_gpadl(channel, &channel->ringbuffer_gpadlhandle);
>  		if (ret) {
>  			pr_err("Close failed: teardown gpadl return %d\n", ret);
>  			/*
> @@ -944,7 +970,7 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
>  			 */
>  		}
> 
> -		channel->ringbuffer_gpadlhandle = 0;
> +		channel->ringbuffer_gpadlhandle.gpadl_handle = 0;
>  	}
> 
>  	if (!ret)
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index bc48855dff10..315278a7cf88 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -1075,14 +1075,15 @@ struct netvsc_device {
>  	/* Receive buffer allocated by us but manages by NetVSP */
>  	void *recv_buf;
>  	u32 recv_buf_size; /* allocated bytes */
> -	u32 recv_buf_gpadl_handle;
> +	struct vmbus_gpadl recv_buf_gpadl_handle;
>  	u32 recv_section_cnt;
>  	u32 recv_section_size;
>  	u32 recv_completion_cnt;
> 
>  	/* Send buffer allocated by us */
>  	void *send_buf;
> -	u32 send_buf_gpadl_handle;
> +	u32 send_buf_size;
> +	struct vmbus_gpadl send_buf_gpadl_handle;
>  	u32 send_section_cnt;
>  	u32 send_section_size;
>  	unsigned long *send_section_map;
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 7bd935412853..1f87e570ed2b 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -278,9 +278,9 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
>  {
>  	int ret;
> 
> -	if (net_device->recv_buf_gpadl_handle) {
> +	if (net_device->recv_buf_gpadl_handle.gpadl_handle) {
>  		ret = vmbus_teardown_gpadl(device->channel,
> -					   net_device->recv_buf_gpadl_handle);
> +					   &net_device->recv_buf_gpadl_handle);
> 
>  		/* If we failed here, we might as well return and have a leak
>  		 * rather than continue and a bugchk
> @@ -290,7 +290,7 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
>  				   "unable to teardown receive buffer's gpadl\n");
>  			return;
>  		}
> -		net_device->recv_buf_gpadl_handle = 0;
> +		net_device->recv_buf_gpadl_handle.gpadl_handle = 0;
>  	}
>  }
> 
> @@ -300,9 +300,9 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device,
>  {
>  	int ret;
> 
> -	if (net_device->send_buf_gpadl_handle) {
> +	if (net_device->send_buf_gpadl_handle.gpadl_handle) {
>  		ret = vmbus_teardown_gpadl(device->channel,
> -					   net_device->send_buf_gpadl_handle);
> +					   &net_device->send_buf_gpadl_handle);
> 
>  		/* If we failed here, we might as well return and have a leak
>  		 * rather than continue and a bugchk
> @@ -312,7 +312,7 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device,
>  				   "unable to teardown send buffer's gpadl\n");
>  			return;
>  		}
> -		net_device->send_buf_gpadl_handle = 0;
> +		net_device->send_buf_gpadl_handle.gpadl_handle = 0;
>  	}
>  }
> 
> @@ -380,7 +380,7 @@ static int netvsc_init_buf(struct hv_device *device,
>  	memset(init_packet, 0, sizeof(struct nvsp_message));
>  	init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_RECV_BUF;
>  	init_packet->msg.v1_msg.send_recv_buf.
> -		gpadl_handle = net_device->recv_buf_gpadl_handle;
> +		gpadl_handle = net_device->recv_buf_gpadl_handle.gpadl_handle;
>  	init_packet->msg.v1_msg.
>  		send_recv_buf.id = NETVSC_RECEIVE_BUFFER_ID;
> 
> @@ -463,6 +463,7 @@ static int netvsc_init_buf(struct hv_device *device,
>  		ret = -ENOMEM;
>  		goto cleanup;
>  	}
> +	net_device->send_buf_size = buf_size;
> 
>  	/* Establish the gpadl handle for this buffer on this
>  	 * channel.  Note: This call uses the vmbus connection rather
> @@ -482,7 +483,7 @@ static int netvsc_init_buf(struct hv_device *device,
>  	memset(init_packet, 0, sizeof(struct nvsp_message));
>  	init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_SEND_BUF;
>  	init_packet->msg.v1_msg.send_send_buf.gpadl_handle =
> -		net_device->send_buf_gpadl_handle;
> +		net_device->send_buf_gpadl_handle.gpadl_handle;
>  	init_packet->msg.v1_msg.send_send_buf.id = NETVSC_SEND_BUFFER_ID;
> 
>  	trace_nvsp_send(ndev, init_packet);
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 652fe2547587..548243dcd895 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -58,11 +58,11 @@ struct hv_uio_private_data {
>  	atomic_t refcnt;
> 
>  	void	*recv_buf;
> -	u32	recv_gpadl;
> +	struct vmbus_gpadl recv_gpadl;
>  	char	recv_name[32];	/* "recv_4294967295" */
> 
>  	void	*send_buf;
> -	u32	send_gpadl;
> +	struct vmbus_gpadl send_gpadl;
>  	char	send_name[32];
>  };
> 
> @@ -179,15 +179,15 @@ hv_uio_new_channel(struct vmbus_channel *new_sc)
>  static void
>  hv_uio_cleanup(struct hv_device *dev, struct hv_uio_private_data *pdata)
>  {
> -	if (pdata->send_gpadl) {
> -		vmbus_teardown_gpadl(dev->channel, pdata->send_gpadl);
> -		pdata->send_gpadl = 0;
> +	if (pdata->send_gpadl.gpadl_handle) {
> +		vmbus_teardown_gpadl(dev->channel, &pdata->send_gpadl);
> +		pdata->send_gpadl.gpadl_handle = 0;
>  		vfree(pdata->send_buf);
>  	}
> 
> -	if (pdata->recv_gpadl) {
> -		vmbus_teardown_gpadl(dev->channel, pdata->recv_gpadl);
> -		pdata->recv_gpadl = 0;
> +	if (pdata->recv_gpadl.gpadl_handle) {
> +		vmbus_teardown_gpadl(dev->channel, &pdata->recv_gpadl);
> +		pdata->recv_gpadl.gpadl_handle = 0;
>  		vfree(pdata->recv_buf);
>  	}
>  }
> @@ -303,7 +303,7 @@ hv_uio_probe(struct hv_device *dev,
> 
>  	/* put Global Physical Address Label in name */
>  	snprintf(pdata->recv_name, sizeof(pdata->recv_name),
> -		 "recv:%u", pdata->recv_gpadl);
> +		 "recv:%u", pdata->recv_gpadl.gpadl_handle);
>  	pdata->info.mem[RECV_BUF_MAP].name = pdata->recv_name;
>  	pdata->info.mem[RECV_BUF_MAP].addr
>  		= (uintptr_t)pdata->recv_buf;
> @@ -324,7 +324,7 @@ hv_uio_probe(struct hv_device *dev,
>  	}
> 
>  	snprintf(pdata->send_name, sizeof(pdata->send_name),
> -		 "send:%u", pdata->send_gpadl);
> +		 "send:%u", pdata->send_gpadl.gpadl_handle);
>  	pdata->info.mem[SEND_BUF_MAP].name = pdata->send_name;
>  	pdata->info.mem[SEND_BUF_MAP].addr
>  		= (uintptr_t)pdata->send_buf;
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index ddc8713ce57b..a9e0bc3b1511 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -803,6 +803,12 @@ struct vmbus_device {
> 
>  #define VMBUS_DEFAULT_MAX_PKT_SIZE 4096
> 
> +struct vmbus_gpadl {
> +	u32 gpadl_handle;
> +	u32 size;
> +	void *buffer;
> +};
> +
>  struct vmbus_channel {
>  	struct list_head listentry;
> 
> @@ -822,7 +828,7 @@ struct vmbus_channel {
>  	bool rescind_ref; /* got rescind msg, got channel reference */
>  	struct completion rescind_event;
> 
> -	u32 ringbuffer_gpadlhandle;
> +	struct vmbus_gpadl ringbuffer_gpadlhandle;
> 
>  	/* Allocated memory for ring buffer */
>  	struct page *ringbuffer_page;
> @@ -1192,10 +1198,10 @@ extern int vmbus_sendpacket_mpb_desc(struct vmbus_channel *channel,
>  extern int vmbus_establish_gpadl(struct vmbus_channel *channel,
>  				      void *kbuffer,
>  				      u32 size,
> -				      u32 *gpadl_handle);
> +				      struct vmbus_gpadl *gpadl);
> 
>  extern int vmbus_teardown_gpadl(struct vmbus_channel *channel,
> -				     u32 gpadl_handle);
> +				     struct vmbus_gpadl *gpadl);
> 
>  void vmbus_reset_channel_cb(struct vmbus_channel *channel);
> 
> --
> 2.25.1