Skip to contentSkip to conversation

Drivers: hv: vmbus: Some miscellaneous fixes

PATCH In review K. Y. Srinivasan 3 participants 6 messages Jun 3, 2016 – Jul 29, 2016 Hyper-V/Azure core and drivers

lore
Share & tools
lore

K. Y. Srinivasan

author
#0 lore raw

cover Drivers: hv: vmbus: Some miscellaneous fixes

Some miscellaneous fixes.

Vitaly Kuznetsov (3):
Drivers: hv: avoid vfree() on crash
Drivers: hv: get rid of redundant messagecount in
create_gpadl_header()
Drivers: hv: don't leak memory in vmbus_establish_gpadl()

 drivers/hv/channel.c      |   44 +++++++++++++++++++++-----------------------
 drivers/hv/hv.c           |    8 +++++---
 drivers/hv/hyperv_vmbus.h |    2 +-
 drivers/hv/vmbus_drv.c    |    8 ++++----
 4 files changed, 31 insertions(+), 31 deletions(-)
Signature · 1 line
1.7.4.1
author
#1 lore raw

PATCH 1/3 Drivers: hv: avoid vfree() on crash

From: Vitaly Kuznetsov <[email protected]>

When we crash from NMI context (e.g. after NMI injection from host when 'sysctl -w kernel.unknown_nmi_panic=1' is set) we hit

kernel BUG at mm/vmalloc.c:1530!

as vfree() is denied. While the issue could be solved with in_nmi() check instead I opted for skipping vfree on all sorts of crashes to reduce the amount of work which can cause consequent crashes. We don't really need to free anything on crash.

Signed-off-by Vitaly Kuznetsov <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv.c           |    8 +++++---
 drivers/hv/hyperv_vmbus.h |    2 +-
 drivers/hv/vmbus_drv.c    |    8 ++++----
 3 files changed, 10 insertions(+), 8 deletions(-)
drivers/hv/hv.c +5 -3
index a1c086b..60dbd6c 100644
@@ -278,7 +278,7 @@ cleanup:
  *
  * This routine is called normally during driver unloading or exiting.
  */
-void hv_cleanup(void)
+void hv_cleanup(bool crash)
 {
 	union hv_x64_msr_hypercall_contents hypercall_msr;
 

@@ -288,7 +288,8 @@ void hv_cleanup(void)
 	if (hv_context.hypercall_page) {
 		hypercall_msr.as_uint64 = 0;
 		wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
-		vfree(hv_context.hypercall_page);
+		if (!crash)
+			vfree(hv_context.hypercall_page);
 		hv_context.hypercall_page = NULL;
 	}
 

@@ -308,7 +309,8 @@ void hv_cleanup(void)
 
 		hypercall_msr.as_uint64 = 0;
 		wrmsrl(HV_X64_MSR_REFERENCE_TSC, hypercall_msr.as_uint64);
-		vfree(hv_context.tsc_page);
+		if (!crash)
+			vfree(hv_context.tsc_page);
 		hv_context.tsc_page = NULL;
 	}
 #endif
drivers/hv/hyperv_vmbus.h +1 -1
index 718b5c7..dfa9fac 100644
@@ -495,7 +495,7 @@ struct hv_ring_buffer_debug_info {
 
 extern int hv_init(void);
 
-extern void hv_cleanup(void);
+extern void hv_cleanup(bool crash);
 
 extern int hv_post_message(union hv_connection_id connection_id,
 			 enum hv_message_type message_type,
drivers/hv/vmbus_drv.c +4 -4
index 952f20f..d11690e 100644
@@ -871,7 +871,7 @@ err_alloc:
 	bus_unregister(&hv_bus);
 
 err_cleanup:
-	hv_cleanup();
+	hv_cleanup(false);
 
 	return ret;
 }

@@ -1323,7 +1323,7 @@ static void hv_kexec_handler(void)
 	vmbus_initiate_unload(false);
 	for_each_online_cpu(cpu)
 		smp_call_function_single(cpu, hv_synic_cleanup, NULL, 1);
-	hv_cleanup();
+	hv_cleanup(false);
 };
 
 static void hv_crash_handler(struct pt_regs *regs)

@@ -1335,7 +1335,7 @@ static void hv_crash_handler(struct pt_regs *regs)
 	 * for kdump.
 	 */
 	hv_synic_cleanup(NULL);
-	hv_cleanup();
+	hv_cleanup(true);
 };
 
 static int __init hv_acpi_init(void)

@@ -1395,7 +1395,7 @@ static void __exit vmbus_exit(void)
 						 &hyperv_panic_block);
 	}
 	bus_unregister(&hv_bus);
-	hv_cleanup();
+	hv_cleanup(false);
 	for_each_online_cpu(cpu) {
 		tasklet_kill(hv_context.event_dpc[cpu]);
 		smp_call_function_single(cpu, hv_synic_cleanup, NULL, 1);
Signature · 1 line
1.7.4.1
author
#2 lore raw

PATCH 2/3 Drivers: hv: get rid of redundant messagecount in create_gpadl_header()

From: Vitaly Kuznetsov <[email protected]>

We use messagecount only once in vmbus_establish_gpadl() to check if it is safe to iterate through the submsglist. We can just initialize the list header in all cases in create_gpadl_header() instead.

Signed-off-by Vitaly Kuznetsov <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel.c |   38 ++++++++++++++++----------------------
 1 files changed, 16 insertions(+), 22 deletions(-)
drivers/hv/channel.c +16 -22
index 56dd261..2b109e8 100644
@@ -238,8 +238,7 @@ EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request);
  * create_gpadl_header - Creates a gpadl for the specified buffer
  */
 static int create_gpadl_header(void *kbuffer, u32 size,
-					 struct vmbus_channel_msginfo **msginfo,
-					 u32 *messagecount)
+			       struct vmbus_channel_msginfo **msginfo)
 {
 	int i;
 	int pagecount;

@@ -283,7 +282,6 @@ static int create_gpadl_header(void *kbuffer, u32 size,
 			gpadl_header->range[0].pfn_array[i] = slow_virt_to_phys(
 				kbuffer + PAGE_SIZE * i) >> PAGE_SHIFT;
 		*msginfo = msgheader;
-		*messagecount = 1;
 
 		pfnsum = pfncount;
 		pfnleft = pagecount - pfncount;

@@ -323,7 +321,6 @@ static int create_gpadl_header(void *kbuffer, u32 size,
 			}
 
 			msgbody->msgsize = msgsize;
-			(*messagecount)++;
 			gpadl_body =
 				(struct vmbus_channel_gpadl_body *)msgbody->msg;
 

@@ -352,6 +349,8 @@ static int create_gpadl_header(void *kbuffer, u32 size,
 		msgheader = kzalloc(msgsize, GFP_KERNEL);
 		if (msgheader == NULL)
 			goto nomem;
+
+		INIT_LIST_HEAD(&msgheader->submsglist);
 		msgheader->msgsize = msgsize;
 
 		gpadl_header = (struct vmbus_channel_gpadl_header *)

@@ -366,7 +365,6 @@ static int create_gpadl_header(void *kbuffer, u32 size,
 				kbuffer + PAGE_SIZE * i) >> PAGE_SHIFT;
 
 		*msginfo = msgheader;
-		*messagecount = 1;
 	}
 
 	return 0;

@@ -391,7 +389,6 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	struct vmbus_channel_gpadl_body *gpadl_body;
 	struct vmbus_channel_msginfo *msginfo = NULL;
 	struct vmbus_channel_msginfo *submsginfo;
-	u32 msgcount;
 	struct list_head *curr;
 	u32 next_gpadl_handle;
 	unsigned long flags;

@@ -400,7 +397,7 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	next_gpadl_handle =
 		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
 
-	ret = create_gpadl_header(kbuffer, size, &msginfo, &msgcount);
+	ret = create_gpadl_header(kbuffer, size, &msginfo);
 	if (ret)
 		return ret;
 

@@ -423,24 +420,21 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	if (ret != 0)
 		goto cleanup;
 
-	if (msgcount > 1) {
-		list_for_each(curr, &msginfo->submsglist) {
+	list_for_each(curr, &msginfo->submsglist) {
+		submsginfo = (struct vmbus_channel_msginfo *)curr;
+		gpadl_body =
+			(struct vmbus_channel_gpadl_body *)submsginfo->msg;
 
-			submsginfo = (struct vmbus_channel_msginfo *)curr;
-			gpadl_body =
-			     (struct vmbus_channel_gpadl_body *)submsginfo->msg;
+		gpadl_body->header.msgtype =
+			CHANNELMSG_GPADL_BODY;
+		gpadl_body->gpadl = next_gpadl_handle;
 
-			gpadl_body->header.msgtype =
-				CHANNELMSG_GPADL_BODY;
-			gpadl_body->gpadl = next_gpadl_handle;
+		ret = vmbus_post_msg(gpadl_body,
+				     submsginfo->msgsize -
+				     sizeof(*submsginfo));
+		if (ret != 0)
+			goto cleanup;
 
-			ret = vmbus_post_msg(gpadl_body,
-					       submsginfo->msgsize -
-					       sizeof(*submsginfo));
-			if (ret != 0)
-				goto cleanup;
-
-		}
 	}
 	wait_for_completion(&msginfo->waitevent);
Signature · 1 line
1.7.4.1
author
#3 lore raw

PATCH 3/3 Drivers: hv: don't leak memory in vmbus_establish_gpadl()

In reply to K. Y. Srinivasan

From: Vitaly Kuznetsov <[email protected]>

In some cases create_gpadl_header() allocates submessages but we never free them.

Signed-off-by Vitaly Kuznetsov <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/channel.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
drivers/hv/channel.c +5 -1
index 2b109e8..a68830c 100644
@@ -388,7 +388,7 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	struct vmbus_channel_gpadl_header *gpadlmsg;
 	struct vmbus_channel_gpadl_body *gpadl_body;
 	struct vmbus_channel_msginfo *msginfo = NULL;
-	struct vmbus_channel_msginfo *submsginfo;
+	struct vmbus_channel_msginfo *submsginfo, *tmp;
 	struct list_head *curr;
 	u32 next_gpadl_handle;
 	unsigned long flags;

@@ -445,6 +445,10 @@ cleanup:
 	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
 	list_del(&msginfo->msglistentry);
 	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
+	list_for_each_entry_safe(submsginfo, tmp, &msginfo->submsglist,
+				 msglistentry) {
+		kfree(submsginfo);
+	}
 
 	kfree(msginfo);
 	return ret;
Signature · 1 line
1.7.4.1
#4 lore raw
In reply to K. Y. Srinivasan
Quoted 7 lines · K. Y. Srinivasan

Some miscellaneous fixes.

Vitaly Kuznetsov (3):
Drivers: hv: avoid vfree() on crash
Drivers: hv: get rid of redundant messagecount in
create_gpadl_header()
Drivers: hv: don't leak memory in vmbus_establish_gpadl()

Greg, K. Y.,

I'm sorry for the ping bug this series was sent a while ago and I'm starting to get worried it'll miss 4.8 as I don't see it in char-misc-next.

There is a bunch of other series waiting to get merged, I'm aware of:

"[PATCH 0/3] Drivers: hv: vmbus: Miscellaneous adjustments" "[PATCH 0/4] Drivers: hv: vmbus: Make in-place consumption always possible"
"[PATCH 1/1] Drivers: hv: Introduce a policy for controlling channel affinity"
"[PATCH 1/1] Tools: hv: kvp: ensure kvp device fd is closed on exec"

I'm sure there are more. Please take a look.

Thanks,

Signature · 1 line
  Vitaly
#5 lore raw
Quoted 18 lines · Vitaly Kuznetsov
Quoted 7 lines · K. Y. Srinivasan

Some miscellaneous fixes.

Vitaly Kuznetsov (3):
Drivers: hv: avoid vfree() on crash
Drivers: hv: get rid of redundant messagecount in
create_gpadl_header()
Drivers: hv: don't leak memory in vmbus_establish_gpadl()

Greg, K. Y.,

I'm sorry for the ping bug this series was sent a while ago and I'm starting to get worried it'll miss 4.8 as I don't see it in char-misc-next.

There is a bunch of other series waiting to get merged, I'm aware of:

Quoted 8 lines

"[PATCH 0/3] Drivers: hv: vmbus: Miscellaneous adjustments" "[PATCH 0/4] Drivers: hv: vmbus: Make in-place consumption always possible"
"[PATCH 1/1] Drivers: hv: Introduce a policy for controlling channel affinity"
"[PATCH 1/1] Tools: hv: kvp: ensure kvp device fd is closed on exec"

I'm sure there are more. Please take a look.

I see lots of hv patches in my to-apply queue, that will be processed after 4.8-rc1 is out. They came in too late to make 4.8, sorry, due to vacation and travel on my part.

thanks,

greg k-h