RE: [PATCH V4 04/13] hyperv: Mark vmbus ring buffer visible to host in Isolation VM
From: Michael Kelley <hidden>
Date: 2021-09-02 00:17:43
Also in:
linux-arch, linux-hyperv, linux-iommu, linux-scsi, netdev, xen-devel
Subsystem:
hyper-v/azure core and drivers, networking drivers, the rest, userspace i/o (uio) · Maintainers:
"K. Y. Srinivasan", Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds, Greg Kroah-Hartman
From: Tianyu Lan <redacted> Sent: Friday, August 27, 2021 10:21 AM
quoted hunk ↗ jump to hunk
Mark vmbus ring buffer visible with set_memory_decrypted() when establish gpadl handle. Signed-off-by: Tianyu Lan <redacted> --- 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 | 36 ++++++++++++++++++++++++++++----- drivers/net/hyperv/hyperv_net.h | 1 + drivers/net/hyperv/netvsc.c | 16 +++++++++++---- drivers/uio/uio_hv_generic.c | 14 +++++++++++-- include/linux/hyperv.h | 8 +++++++- 5 files changed, 63 insertions(+), 12 deletions(-)diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index f3761c73b074..82650beb3af0 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>@@ -474,6 +475,13 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, if (ret) return ret; + ret = set_memory_decrypted((unsigned long)kbuffer, + HVPFN_UP(size)); + if (ret) { + pr_warn("Failed to set host visibility for new GPADL %d.\n", ret); + return ret; + } + init_completion(&msginfo->waitevent); msginfo->waiting_channel = channel;@@ -549,6 +557,11 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, } kfree(msginfo); + + if (ret) + set_memory_encrypted((unsigned long)kbuffer, + HVPFN_UP(size)); + return ret; }@@ -639,6 +652,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; u32 send_pages, recv_pages; unsigned long flags; int err;@@ -759,7 +773,10 @@ static int __vmbus_open(struct vmbus_channel *newchannel, error_free_info: kfree(open_info); error_free_gpadl: - vmbus_teardown_gpadl(newchannel, newchannel->ringbuffer_gpadlhandle); + gpadl.gpadl_handle = newchannel->ringbuffer_gpadlhandle; + gpadl.buffer = page_address(newchannel->ringbuffer_page); + gpadl.size = (send_pages + recv_pages) << PAGE_SHIFT; + vmbus_teardown_gpadl(newchannel, &gpadl); newchannel->ringbuffer_gpadlhandle = 0; error_clean_ring: hv_ringbuffer_cleanup(&newchannel->outbound);@@ -806,7 +823,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 +842,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 +876,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)); + 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 +919,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; int ret; vmbus_reset_channel_cb(channel);@@ -934,8 +958,10 @@ 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); + gpadl.gpadl_handle = channel->ringbuffer_gpadlhandle; + gpadl.buffer = page_address(channel->ringbuffer_page); + gpadl.size = channel->ringbuffer_pagecount; + ret = vmbus_teardown_gpadl(channel, &gpadl); if (ret) { pr_err("Close failed: teardown gpadl return %d\n", ret); /*diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index bc48855dff10..aa7c9962dbd8 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h@@ -1082,6 +1082,7 @@ struct netvsc_device { /* Send buffer allocated by us */ void *send_buf; + u32 send_buf_size; u32 send_buf_gpadl_handle; u32 send_section_cnt; u32 send_section_size;diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 7bd935412853..f19bffff6a63 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c@@ -276,11 +276,14 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device, struct netvsc_device *net_device, struct net_device *ndev) { + struct vmbus_gpadl gpadl; int ret; if (net_device->recv_buf_gpadl_handle) { - ret = vmbus_teardown_gpadl(device->channel, - net_device->recv_buf_gpadl_handle); + gpadl.gpadl_handle = net_device->recv_buf_gpadl_handle; + gpadl.buffer = net_device->recv_buf; + gpadl.size = net_device->recv_buf_size; + ret = vmbus_teardown_gpadl(device->channel, &gpadl); /* If we failed here, we might as well return and have a leak * rather than continue and a bugchk@@ -298,11 +301,15 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device, struct netvsc_device *net_device, struct net_device *ndev) { + struct vmbus_gpadl gpadl; int ret; if (net_device->send_buf_gpadl_handle) { - ret = vmbus_teardown_gpadl(device->channel, - net_device->send_buf_gpadl_handle); + gpadl.gpadl_handle = net_device->send_buf_gpadl_handle; + gpadl.buffer = net_device->send_buf; + gpadl.size = net_device->send_buf_size; + + ret = vmbus_teardown_gpadl(device->channel, &gpadl); /* If we failed here, we might as well return and have a leak * rather than continue and a bugchk@@ -463,6 +470,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 ratherdiff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c index 652fe2547587..13c5df8dd11d 100644 --- a/drivers/uio/uio_hv_generic.c +++ b/drivers/uio/uio_hv_generic.c@@ -179,14 +179,24 @@ hv_uio_new_channel(struct vmbus_channel *new_sc) static void hv_uio_cleanup(struct hv_device *dev, struct hv_uio_private_data *pdata) { + struct vmbus_gpadl gpadl; + if (pdata->send_gpadl) { - vmbus_teardown_gpadl(dev->channel, pdata->send_gpadl); + gpadl.gpadl_handle = pdata->send_gpadl; + gpadl.buffer = pdata->send_buf; + gpadl.size = SEND_BUFFER_SIZE; + + vmbus_teardown_gpadl(dev->channel, &gpadl); pdata->send_gpadl = 0; vfree(pdata->send_buf); } if (pdata->recv_gpadl) { - vmbus_teardown_gpadl(dev->channel, pdata->recv_gpadl); + gpadl.gpadl_handle = pdata->recv_gpadl; + gpadl.buffer = pdata->recv_buf; + gpadl.size = RECV_BUFFER_SIZE; + + vmbus_teardown_gpadl(dev->channel, &gpadl); pdata->recv_gpadl = 0; vfree(pdata->recv_buf); }diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index ddc8713ce57b..757e09606fd3 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;@@ -1195,7 +1201,7 @@ extern int vmbus_establish_gpadl(struct vmbus_channel *channel, u32 *gpadl_handle); 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
This isn't quite what I had in mind in my comments on v3 of this patch series. My idea is to store the full struct vmbus_gpadl data structure in places where previously just the u32 gpadl_handle was stored. Then pass around a pointer to the struct vmbus_gpadl where previously just the gpadl_handle (or a pointer to it) was passed. This lets __vmbus_establish_gpadl() fill in the actual handle value as well the other info (buffer pointer and size) that vmbus_teardown_gpadl() needs. Callers of the gpadl functions don't need to worry about saving or finding the right info. Most of the changes are just tweaking the references to what is now a struct instead of a u32. Here's a diff of what I had in mind. My version also has vmbus_teardown_gpadl() set the handle field to zero, rather than each caller having to do it. The code compiles, but I have not done a runtime test. This diff is a net +21 lines of code, whereas your v3 and v4 patches were both +51 lines of code.
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index f3761c7..fc041ae 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_handle) { struct vmbus_channel_gpadl_header *gpadlmsg; struct vmbus_channel_gpadl_body *gpadl_body;
@@ -474,6 +475,13 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, if (ret) return ret; + ret = set_memory_decrypted((unsigned long)kbuffer, + HVPFN_UP(size)); + if (ret) { + pr_warn("Failed to set host visibility for new GPADL %d.\n", ret); + return ret; + } + init_completion(&msginfo->waitevent); msginfo->waiting_channel = channel;
@@ -537,7 +545,9 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, } /* At this point, we received the gpadl created msg */ - *gpadl_handle = gpadlmsg->gpadl; + gpadl_handle->handle = gpadlmsg->gpadl; + gpadl_handle->buffer = kbuffer; + gpadl_handle->size = size; cleanup: spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
@@ -549,6 +559,11 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, } kfree(msginfo); + + if (ret) + set_memory_encrypted((unsigned long)kbuffer, + HVPFN_UP(size)); + return ret; }
@@ -561,7 +576,7 @@ 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_handle) { return __vmbus_establish_gpadl(channel, HV_GPADL_BUFFER, kbuffer, size, 0U, gpadl_handle);
@@ -675,7 +690,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.handle = 0; err = __vmbus_establish_gpadl(newchannel, HV_GPADL_RING, page_address(newchannel->ringbuffer_page),
@@ -701,7 +716,7 @@ 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.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 +774,7 @@ 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); error_clean_ring: hv_ringbuffer_cleanup(&newchannel->outbound); hv_ringbuffer_cleanup(&newchannel->inbound);
@@ -806,7 +820,7 @@ int vmbus_open(struct vmbus_channel *newchannel, /* * 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 +839,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->handle; spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags); list_add_tail(&info->msglistentry,
@@ -844,6 +858,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle) goto post_msg_err; wait_for_completion(&info->waitevent); + gpadl->handle = 0; post_msg_err: /*
@@ -859,6 +874,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)); + if (ret) + pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret); + return ret; } EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
@@ -933,9 +954,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel) } /* Tear down the gpadl for the channel's ring buffer */ - else if (channel->ringbuffer_gpadlhandle) { + else if (channel->ringbuffer_gpadlhandle.handle) { ret = vmbus_teardown_gpadl(channel, - channel->ringbuffer_gpadlhandle); + &channel->ringbuffer_gpadlhandle); if (ret) { pr_err("Close failed: teardown gpadl return %d\n", ret); /*
@@ -943,8 +964,6 @@ static int vmbus_close_internal(struct vmbus_channel *channel) * it is perhaps better to leak memory. */ } - - channel->ringbuffer_gpadlhandle = 0; } if (!ret)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index bc48855..54cbce1 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h@@ -1075,14 +1075,14 @@ 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; + 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 7bd9354..585974c 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.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,6 @@ 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; } }
@@ -300,9 +299,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.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 +311,6 @@ 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; } }
@@ -380,7 +378,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.handle; init_packet->msg.v1_msg. send_recv_buf.id = NETVSC_RECEIVE_BUFFER_ID;
@@ -482,7 +480,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.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 652fe25..97e08e7 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,13 @@ static int hv_uio_ring_mmap(struct file *filp, struct kobject *kobj, 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.handle) { + vmbus_teardown_gpadl(dev->channel, &pdata->send_gpadl); vfree(pdata->send_buf); } - if (pdata->recv_gpadl) { - vmbus_teardown_gpadl(dev->channel, pdata->recv_gpadl); - pdata->recv_gpadl = 0; + if (pdata->recv_gpadl.handle) { + vmbus_teardown_gpadl(dev->channel, &pdata->recv_gpadl); vfree(pdata->recv_buf); } }
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 2e859d2..a0d64c3 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h@@ -809,6 +809,12 @@ struct vmbus_device { #define VMBUS_DEFAULT_MAX_PKT_SIZE 4096 +struct vmbus_gpadl { + u32 handle; + u32 size; + void *buffer; +}; + struct vmbus_channel { struct list_head listentry;
@@ -828,7 +834,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;
@@ -1208,10 +1214,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_handle); 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);