Drivers: hv: Miscellaneous fixes

PATCH In review [email protected] 8 participants 18 messages Oct 18, 2018 – Nov 11, 2018

Subsystems (2)

Stable branch Hyper-V/Azure core and drivers

lore

Share & tools

lore

[email protected]

author

2018-10-18 05:20 UTC #0 lore raw

v2 cover Drivers: hv: Miscellaneous fixes

From: "K. Y. Srinivasan" <[email protected]>

Miscellaneous fixes.

V2: Addressed comments from Greg.

Dexuan Cui (3):
Drivers: hv: kvp: Fix the recent regression caused by incorrect
clean-up
Drivers: hv: kvp: Use %u to print U32
Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1

Haiyang Zhang (1):
hv_utils: update name in struct hv_driver util_drv

K. Y. Srinivasan (1):
Drivers: hv: vmbus: Get rid of unnecessary state in hv_context

 drivers/hv/hv.c           | 10 +++-------
 drivers/hv/hv_kvp.c       | 28 +++++++++++++++++++++++-----
 drivers/hv/hv_util.c      |  2 +-
 drivers/hv/hyperv_vmbus.h |  2 --
 tools/hv/hv_kvp_daemon.c  | 15 +++++++++++++--
 5 files changed, 40 insertions(+), 17 deletions(-)

Signature · 1 line

2.18.0

[email protected]

author

2018-10-18 05:10 UTC #1 lore raw

PATCH v2 1/5 Drivers: hv: vmbus: Get rid of unnecessary state in hv_context

From: "K. Y. Srinivasan" <[email protected]>

Currently we are replicating state in struct hv_context that is unnecessary - this state can be retrieved from the hypervisor. Furthermore, this is a per-cpu state that is being maintained as a global state in struct hv_context. Get rid of this state in struct hv_context.

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/hv/hv.c           | 10 +++-------
 drivers/hv/hyperv_vmbus.h |  2 --
 2 files changed, 3 insertions(+), 9 deletions(-)

drivers/hv/hv.c +3 -7

index 332d7c34be5c..166c2501de17 100644
@@ -33,9 +33,7 @@
 #include "hyperv_vmbus.h"
 
 /* The one and only */
-struct hv_context hv_context = {
-	.synic_initialized	= false,
-};
+struct hv_context hv_context;
 
 /*
  * If false, we're using the old mechanism for stimer0 interrupts

@@ -326,8 +324,6 @@ int hv_synic_init(unsigned int cpu)
 
 	hv_set_synic_state(sctrl.as_uint64);
 
-	hv_context.synic_initialized = true;
-
 	/*
 	 * Register the per-cpu clockevent source.
 	 */

@@ -373,7 +369,8 @@ int hv_synic_cleanup(unsigned int cpu)
 	bool channel_found = false;
 	unsigned long flags;
 
-	if (!hv_context.synic_initialized)
+	hv_get_synic_state(sctrl.as_uint64);
+	if (sctrl.enable != 1)
 		return -EFAULT;
 
 	/*

@@ -435,7 +432,6 @@ int hv_synic_cleanup(unsigned int cpu)
 	hv_set_siefp(siefp.as_uint64);
 
 	/* Disable the global synic bit */
-	hv_get_synic_state(sctrl.as_uint64);
 	sctrl.enable = 0;
 	hv_set_synic_state(sctrl.as_uint64);

drivers/hv/hyperv_vmbus.h +0 -2

index 72eaba3d50fc..f17c06a5e74b 100644
@@ -230,8 +230,6 @@ struct hv_context {
 
 	void *tsc_page;
 
-	bool synic_initialized;
-
 	struct hv_per_cpu_context __percpu *cpu_context;
 
 	/*

Signature · 1 line

2.18.0

[email protected]

author

2018-10-18 05:10 UTC #2 lore raw

PATCH v2 2/5 hv_utils: update name in struct hv_driver util_drv

From: Haiyang Zhang <[email protected]>

The correct module name is hv_utils. This patch corrects the name in struct hv_driver util_drv.

Signed-off-by Haiyang Zhang <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/hv/hv_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/hv/hv_util.c +1 -1

index 423205077bf6..f10eeb120c8b 100644
@@ -483,7 +483,7 @@ MODULE_DEVICE_TABLE(vmbus, id_table);
 
 /* The one and only one */
 static  struct hv_driver util_drv = {
-	.name = "hv_util",
+	.name = "hv_utils",
 	.id_table = id_table,
 	.probe =  util_probe,
 	.remove =  util_remove,

Signature · 1 line

2.18.0

[email protected]

author

2018-10-18 05:10 UTC #3 lore raw

PATCH v2 3/5 Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

In reply to [email protected]

From: Dexuan Cui <[email protected]>

In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns out the userland hv_kvp_daemon needs the info of operation, adapter_id and addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.

And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size of 0 is passed to the kvp daemon, and the pool files /var/lib/hyperv/.kvp_pool_* can't be updated.

This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.

This patch is tested on WS 2012 R2 and 2016.

Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall through" warnings")

Signed-off-by Dexuan Cui <[email protected]>

Cc K. Y. Srinivasan <[email protected]>

Cc Haiyang Zhang <[email protected]>

Cc Stephen Hemminger <[email protected]>

Cc <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

drivers/hv/hv_kvp.c +22 -4

index a7513a8a8e37..d6106e1a0d4a 100644
@@ -353,6 +353,9 @@ static void process_ib_ipinfo(void *in_msg, void *out_msg, int op)
 
 		out->body.kvp_ip_val.dhcp_enabled = in->kvp_ip_val.dhcp_enabled;
 
+		/* fallthrough */
+
+	case KVP_OP_GET_IP_INFO:
 		utf16s_to_utf8s((wchar_t *)in->kvp_ip_val.adapter_id,
 				MAX_ADAPTER_ID_SIZE,
 				UTF16_LITTLE_ENDIAN,

@@ -405,7 +408,11 @@ kvp_send_key(struct work_struct *dummy)
 		process_ib_ipinfo(in_msg, message, KVP_OP_SET_IP_INFO);
 		break;
 	case KVP_OP_GET_IP_INFO:
-		/* We only need to pass on message->kvp_hdr.operation.  */
+		/*
+		 * We only need to pass on the info of operation, adapter_id
+		 * and addr_family to the userland kvp daemon.
+		 */
+		process_ib_ipinfo(in_msg, message, KVP_OP_GET_IP_INFO);
 		break;
 	case KVP_OP_SET:
 		switch (in_msg->body.kvp_set.data.value_type) {

@@ -446,9 +453,9 @@ kvp_send_key(struct work_struct *dummy)
 
 		}
 
-		break;
-
-	case KVP_OP_GET:
+		/*
+		 * The key is always a string - utf16 encoding.
+		 */
 		message->body.kvp_set.data.key_size =
 			utf16s_to_utf8s(
 			(wchar_t *)in_msg->body.kvp_set.data.key,

@@ -456,6 +463,17 @@ kvp_send_key(struct work_struct *dummy)
 			UTF16_LITTLE_ENDIAN,
 			message->body.kvp_set.data.key,
 			HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
+
+		break;
+
+	case KVP_OP_GET:
+		message->body.kvp_get.data.key_size =
+			utf16s_to_utf8s(
+			(wchar_t *)in_msg->body.kvp_get.data.key,
+			in_msg->body.kvp_get.data.key_size,
+			UTF16_LITTLE_ENDIAN,
+			message->body.kvp_get.data.key,
+			HV_KVP_EXCHANGE_MAX_KEY_SIZE - 1) + 1;
 		break;
 
 	case KVP_OP_DELETE:

Signature · 1 line

2.18.0

Michael Kelley

Reviewed-by on v2 3/5

2018-10-24 15:37 UTC #4 lore raw

RE: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

From: [email protected] <[email protected]> Sent: Wednesday, October 17, 2018 10:10 PM

Quoted 25 lines

From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns out the userland hv_kvp_daemon needs the info of operation, adapter_id and addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size of 0 is passed to the kvp daemon, and the pool files /var/lib/hyperv/.kvp_pool_* can't be updated.
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall through" warnings")
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>

Quoted 4 lines

---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Reviewed-by Michael Kelley <[email protected]>

Dexuan Cui

maintainer

2018-10-31 23:24 UTC #5 lore raw

RE: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

Quoted 34 lines

From: Michael Kelley <[email protected]>
Sent: Wednesday, October 24, 2018 08:38
From: [email protected] <[email protected]> Sent: Wednesday, October 17, 2018 10:10 PM
Quoted 13 lines
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns out the userland hv_kvp_daemon needs the info of operation, adapter_id and addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size of 0 is passed to the kvp daemon, and the pool files /var/lib/hyperv/.kvp_pool_* can't be updated.
Quoted 6 lines
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall
through" warnings")
Quoted 10 lines
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Quoted 1 line

Reviewed-by Michael Kelley <[email protected]>

Hi Greg,
Can you please take a look at this patch?

We need it to fix a regression introduced by me in:

Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall through" warnings")

The faulty patch is being meged into the old stable kernels... So we need to take this patch ASAP. Thanks!

-- Dexuan

[email protected]

maintainer

2018-11-01 18:56 UTC #6 lore raw

Re: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

Quoted 34 lines · Dexuan Cui

Quoted 34 lines
From: Michael Kelley <[email protected]>
Sent: Wednesday, October 24, 2018 08:38
From: [email protected] <[email protected]> Sent: Wednesday, October 17, 2018 10:10 PM
Quoted 13 lines
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns out the userland hv_kvp_daemon needs the info of operation, adapter_id and addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size of 0 is passed to the kvp daemon, and the pool files /var/lib/hyperv/.kvp_pool_* can't be updated.
Quoted 6 lines
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall
through" warnings")
Quoted 10 lines
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Quoted 4 lines

Quoted 1 line
Reviewed-by Michael Kelley <[email protected]>
Hi Greg,
Can you please take a look at this patch?

Nope, I'm not the hv maintainer, they need to look at this and ack it, not me :)

greg k-h

Dexuan Cui

maintainer

2018-11-01 19:22 UTC #7 lore raw

RE: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

Quoted 44 lines

From: [email protected] <[email protected]>
Sent: Thursday, November 1, 2018 11:57
To: Dexuan Cui <[email protected]>
Quoted 3 lines · Dexuan Cui
Quoted 3 lines
From: Michael Kelley <[email protected]>
Sent: Wednesday, October 24, 2018 08:38
From: [email protected] <[email protected]> Sent:
Wednesday,
Quoted 5 lines
Quoted 5 lines
October 17, 2018 10:10 PM
Quoted 4 lines
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns
out
Quoted 1 line
Quoted 1 line
Quoted 1 line
the userland hv_kvp_daemon needs the info of operation, adapter_id
and
Quoted 27 lines
Quoted 26 lines
Quoted 8 lines
addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size of 0 is passed to the kvp daemon, and the pool files /var/lib/hyperv/.kvp_pool_* can't be updated.
Quoted 6 lines
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall
through" warnings")
Quoted 10 lines
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
Reviewed-by Michael Kelley <[email protected]>

Quoted 7 lines

Quoted 2 lines
Hi Greg,
Can you please take a look at this patch?
Nope, I'm not the hv maintainer, they need to look at this and ack it, not me :)
greg k-h

Hi Greg,
KY has added his Signed-off-by in the mail.

I'll ask the other HV maintainers to take a look as well.

Thanks,
-- Dexuan

[email protected]

maintainer

2018-11-02 04:54 UTC #8 lore raw

Re: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

Quoted 50 lines · Dexuan Cui

Quoted 50 lines
From: [email protected] <[email protected]>
Sent: Thursday, November 1, 2018 11:57
To: Dexuan Cui <[email protected]>
Quoted 3 lines · Dexuan Cui
Quoted 3 lines
From: Michael Kelley <[email protected]>
Sent: Wednesday, October 24, 2018 08:38
From: [email protected] <[email protected]> Sent:
Wednesday,
Quoted 5 lines
Quoted 5 lines
October 17, 2018 10:10 PM
Quoted 4 lines
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns
out
Quoted 1 line
Quoted 1 line
Quoted 1 line
the userland hv_kvp_daemon needs the info of operation, adapter_id
and
Quoted 25 lines
Quoted 25 lines
Quoted 8 lines
addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size of 0 is passed to the kvp daemon, and the pool files /var/lib/hyperv/.kvp_pool_* can't be updated.
Quoted 6 lines
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall
through" warnings")
Quoted 10 lines
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
Quoted 4 lines
Quoted 1 line
Reviewed-by Michael Kelley <[email protected]>
Hi Greg,
Can you please take a look at this patch?
Nope, I'm not the hv maintainer, they need to look at this and ack it, not me :)

Quoted 6 lines

Quoted 1 line
greg k-h
Hi Greg,
KY has added his Signed-off-by in the mail.
I'll ask the other HV maintainers to take a look as well.

Ok, then I'll look at it after 4.20-rc1 is out, nothing I can do until then anyway...

thanks,

greg k-h

Dexuan Cui

maintainer Reviewed-by Michael Kelley

2018-11-11 04:18 UTC #9 lore raw

RE: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

Quoted 72 lines

From: [email protected] <[email protected]>
Sent: Thursday, November 1, 2018 21:54
To: Dexuan Cui <[email protected]>
Cc: Michael Kelley <[email protected]>; KY Srinivasan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Stephen Hemminger <[email protected]>; vkuznets <[email protected]>; Sasha Levin <[email protected]>; Haiyang Zhang <[email protected]>; [email protected]
Subject: Re: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up
Quoted 8 lines · Dexuan Cui
Quoted 8 lines
From: [email protected] <[email protected]>
Sent: Thursday, November 1, 2018 11:57
To: Dexuan Cui <[email protected]>
Quoted 3 lines · Dexuan Cui
Quoted 3 lines
From: Michael Kelley <[email protected]>
Sent: Wednesday, October 24, 2018 08:38
From: [email protected] <[email protected]> Sent:
Quoted 6 lines
Quoted 6 lines
Wednesday,
Quoted 5 lines
Quoted 5 lines
October 17, 2018 10:10 PM
Quoted 4 lines
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it
turns
Quoted 8 lines
Quoted 8 lines
out
Quoted 1 line
Quoted 1 line
Quoted 1 line
the userland hv_kvp_daemon needs the info of operation, adapter_id
and
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size
of
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
0 is passed to the kvp daemon, and the pool files
/var/lib/hyperv/.kvp_pool_* can't be updated.
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may
fall
Quoted 24 lines
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
through" warnings")
Quoted 10 lines
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
Reviewed-by Michael Kelley <[email protected]>
Quoted 7 lines
Quoted 2 lines
Hi Greg,
Can you please take a look at this patch?
Nope, I'm not the hv maintainer, they need to look at this and ack it, not me :)
greg k-h
Hi Greg,
KY has added his Signed-off-by in the mail.

Quoted 8 lines

Quoted 1 line
I'll ask the other HV maintainers to take a look as well.
Ok, then I'll look at it after 4.20-rc1 is out, nothing I can do until then anyway...
thanks,
greg k-h

Hi Greg,
Can you please take a look at the patch now?

The patch has received

Reviewed-by Michael Kelley <[email protected]>

Signed-off-by Haiyang Zhang <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

Thanks,
-- Dexuan

[email protected]

maintainer

2018-11-11 17:09 UTC #10 lore raw

Re: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

Quoted 77 lines · Dexuan Cui

Quoted 77 lines
From: [email protected] <[email protected]>
Sent: Thursday, November 1, 2018 21:54
To: Dexuan Cui <[email protected]>
Cc: Michael Kelley <[email protected]>; KY Srinivasan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Stephen Hemminger <[email protected]>; vkuznets <[email protected]>; Sasha Levin <[email protected]>; Haiyang Zhang <[email protected]>; [email protected]
Subject: Re: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up
Quoted 8 lines · Dexuan Cui
Quoted 8 lines
From: [email protected] <[email protected]>
Sent: Thursday, November 1, 2018 11:57
To: Dexuan Cui <[email protected]>
Quoted 3 lines · Dexuan Cui
Quoted 3 lines
From: Michael Kelley <[email protected]>
Sent: Wednesday, October 24, 2018 08:38
From: [email protected] <[email protected]> Sent:
Quoted 6 lines
Quoted 6 lines
Wednesday,
Quoted 5 lines
Quoted 5 lines
October 17, 2018 10:10 PM
Quoted 4 lines
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it
turns
Quoted 8 lines
Quoted 8 lines
out
Quoted 1 line
Quoted 1 line
Quoted 1 line
the userland hv_kvp_daemon needs the info of operation, adapter_id
and
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size
of
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
0 is passed to the kvp daemon, and the pool files
/var/lib/hyperv/.kvp_pool_* can't be updated.
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may
fall
Quoted 19 lines
Quoted 19 lines
Quoted 11 lines
Quoted 11 lines
through" warnings")
Quoted 10 lines
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
Quoted 4 lines
Quoted 1 line
Reviewed-by Michael Kelley <[email protected]>
Hi Greg,
Can you please take a look at this patch?
Nope, I'm not the hv maintainer, they need to look at this and ack it, not me :)
Quoted 6 lines
Quoted 1 line
greg k-h
Hi Greg,
KY has added his Signed-off-by in the mail.
I'll ask the other HV maintainers to take a look as well.
Ok, then I'll look at it after 4.20-rc1 is out, nothing I can do until then anyway...

Quoted 6 lines

Quoted 3 lines
thanks,
greg k-h
Hi Greg,
Can you please take a look at the patch now?

It's in my queue, sorry, it's big right now due to travel...

KY Srinivasan

2018-11-02 21:55 UTC #11 lore raw

RE: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

In reply to [email protected]

Quoted 27 lines

-----Original Message-----
From: [email protected] <[email protected]>
Sent: Thursday, November 1, 2018 11:57 AM
To: Dexuan Cui <[email protected]>
Cc: Michael Kelley <[email protected]>; KY Srinivasan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Stephen Hemminger <[email protected]>; vkuznets <[email protected]>; Sasha Levin <[email protected]>; Haiyang Zhang <[email protected]>; [email protected]
Subject: Re: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up
Quoted 3 lines · Dexuan Cui
Quoted 3 lines
From: Michael Kelley <[email protected]>
Sent: Wednesday, October 24, 2018 08:38
From: [email protected] <[email protected]> Sent:
Wednesday,
Quoted 5 lines
Quoted 5 lines
October 17, 2018 10:10 PM
Quoted 4 lines
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it
turns out

Quoted 32 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
the userland hv_kvp_daemon needs the info of operation, adapter_id
and
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size
of
Quoted 24 lines
Quoted 20 lines
Quoted 9 lines
0 is passed to the kvp daemon, and the pool files
/var/lib/hyperv/.kvp_pool_* can't be updated.
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.
Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall
through" warnings")
Quoted 10 lines
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
---
 drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
Quoted 1 line
Reviewed-by Michael Kelley <[email protected]>
Hi Greg,
Can you please take a look at this patch?

Greg,

I have already signed-off on this patch.

K. Y

Quoted 5 lines

Nope, I'm not the hv maintainer, they need to look at this and ack it, not me :)
greg k-h

Haiyang Zhang

maintainer

2018-11-01 19:38 UTC #12 lore raw

RE: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

In reply to [email protected]

Quoted 32 lines

-----Original Message-----
From: [email protected] <[email protected]>
Sent: Thursday, October 18, 2018 1:10 AM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Stephen Hemminger <[email protected]>; Michael Kelley <[email protected]>; vkuznets <[email protected]>
Cc: Dexuan Cui <[email protected]>; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; [email protected]
Subject: [PATCH V2 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up
From: Dexuan Cui <[email protected]>
In kvp_send_key(), we do need call process_ib_ipinfo() if message->kvp_hdr.operation is KVP_OP_GET_IP_INFO, because it turns out the userland hv_kvp_daemon needs the info of operation, adapter_id and addr_family. With the incorrect fc62c3b1977d, the host can't get the VM's IP via KVP.
And, fc62c3b1977d added a "break;", but actually forgot to initialize the key_size/value in the case of KVP_OP_SET, so the default key_size of 0 is passed to the kvp daemon, and the pool files /var/lib/hyperv/.kvp_pool_* can't be updated.
This patch effectively rolls back the previous fc62c3b1977d, and correctly fixes the "this statement may fall through" warnings.
This patch is tested on WS 2012 R2 and 2016.

Quoted 8 lines

Fixes fc62c3b1977d ("Drivers: hv: kvp: Fix two "this statement may fall
through" warnings")
Signed-off-by Dexuan Cui <[email protected]>
Cc K. Y. Srinivasan <[email protected]>
Cc Haiyang Zhang <[email protected]>
Cc Stephen Hemminger <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>

Signed-off-by Haiyang Zhang <[email protected]>

Thanks!

[email protected]

author

2018-10-18 05:21 UTC #13 lore raw

PATCH v2 4/5 Drivers: hv: kvp: Use %u to print U32

In reply to [email protected]

From: Dexuan Cui <[email protected]>

I didn't find a real issue. Let's just make it consistent with the next "case REG_U64:" where %llu is used.

Signed-off-by Dexuan Cui <[email protected]>

Cc K. Y. Srinivasan <[email protected]>

Cc Haiyang Zhang <[email protected]>

Cc Stephen Hemminger <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/hv/hv_kvp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/hv/hv_kvp.c +1 -1

index d6106e1a0d4a..5054d1105236 100644
@@ -437,7 +437,7 @@ kvp_send_key(struct work_struct *dummy)
 			val32 = in_msg->body.kvp_set.data.value_u32;
 			message->body.kvp_set.data.value_size =
 				sprintf(message->body.kvp_set.data.value,
-					"%d", val32) + 1;
+					"%u", val32) + 1;
 			break;
 
 		case REG_U64:

Signature · 1 line

2.18.0

[email protected]

author

2018-10-18 05:10 UTC #14 lore raw

PATCH v2 5/5 Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1

In reply to [email protected]

From: Dexuan Cui <[email protected]>

The patch fixes:

hv_kvp_daemon.c: In function 'kvp_set_ip_info':
hv_kvp_daemon.c:1305:2: note: 'snprintf' output between 41 and 4136 bytes into a destination of size 4096

The "(unsigned int)str_len" is to avoid:

hv_kvp_daemon.c:1309:30: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Wsign-compare]

Signed-off-by Dexuan Cui <[email protected]>

Cc K. Y. Srinivasan <[email protected]>

Cc Haiyang Zhang <[email protected]>

Cc Stephen Hemminger <[email protected]>

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 tools/hv/hv_kvp_daemon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

tools/hv/hv_kvp_daemon.c +13 -2

index bbb2a8ef367c..d7e06fe0270e 100644
@@ -1178,6 +1178,7 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
 	FILE *file;
 	char cmd[PATH_MAX];
 	char *mac_addr;
+	int str_len;
 
 	/*
 	 * Set the configuration for the specified interface with

@@ -1301,8 +1302,18 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
 	 * invoke the external script to do its magic.
 	 */
 
-	snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s",
-		 "hv_set_ifconfig", if_file);
+	str_len = snprintf(cmd, sizeof(cmd), KVP_SCRIPTS_PATH "%s %s",
+			   "hv_set_ifconfig", if_file);
+	/*
+	 * This is a little overcautious, but it's necessary to suppress some
+	 * false warnings from gcc 8.0.1.
+	 */
+	if (str_len <= 0 || (unsigned int)str_len >= sizeof(cmd)) {
+		syslog(LOG_ERR, "Cmd '%s' (len=%d) may be too long",
+		       cmd, str_len);
+		return HV_E_FAIL;
+	}
+
 	if (system(cmd)) {
 		syslog(LOG_ERR, "Failed to execute cmd '%s'; error: %d %s",
 				cmd, errno, strerror(errno));

Signature · 1 line

2.18.0

Dan Carpenter

2018-10-18 06:33 UTC #15 lore raw

Re: [PATCH V2 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1

Quoted 4 lines · On Thu, Oct 18, 2018 at 05:09:32AM +0000, [email protected] wrote:

From: Dexuan Cui <[email protected]>
The patch fixes:

Quoted 9 lines

hv_kvp_daemon.c: In function 'kvp_set_ip_info':
hv_kvp_daemon.c:1305:2: note: 'snprintf' output between 41 and 4136 bytes into a destination of size 4096
The "(unsigned int)str_len" is to avoid:
hv_kvp_daemon.c:1309:30: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Wsign-compare]

Ugh... Any tool with the most basic flow analysis would realize this was a false positive. We use at least three static analyzers which catch signedness bugs. Can we turn off GCC's warning on this until they improve it a bit?

regards,
dan carpenter

David Laight

2018-10-18 15:08 UTC #16 lore raw

RE: [PATCH V2 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1

From: Dan Carpenter

Quoted 15 lines

Sent: 18 October 2018 07:33
Quoted 12 lines · On Thu, Oct 18, 2018 at 05:09:32AM +0000, [email protected] wrote:
From: Dexuan Cui <[email protected]>
The patch fixes:
hv_kvp_daemon.c: In function 'kvp_set_ip_info':
hv_kvp_daemon.c:1305:2: note: 'snprintf' output between 41 and 4136 bytes into a destination of size 4096
The "(unsigned int)str_len" is to avoid:
hv_kvp_daemon.c:1309:30: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int' [-Wsign-compare]

I usually use 'str_len + 0u' rather than a cast.

Quoted 4 lines

Ugh... Any tool with the most basic flow analysis would realize this was a false positive. We use at least three static analyzers which catch signedness bugs. Can we turn off GCC's warning on this until they improve it a bit?

Yes, would be nice if it attempted to follow the valid domain of variables.

I recently had to change:

	unsigned char a, b;
	unsigned int c;
	...
	if (a + b < c)

To stop a 'signedness' warning.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Michael Kelley

Reviewed-by on v2 1/5

2018-10-24 15:04 UTC #17 lore raw

RE: [PATCH V2 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context

In reply to [email protected]

From: [email protected] <[email protected]> Sent: Wednesday, October 17, 2018 10:09 PM

Quoted 6 lines

Currently we are replicating state in struct hv_context that is unnecessary - this state can be retrieved from the hypervisor. Furthermore, this is a per-cpu state that is being maintained as a global state in struct hv_context. Get rid of this state in struct hv_context.

Quoted 6 lines

Signed-off-by K. Y. Srinivasan <[email protected]>

---
 drivers/hv/hv.c           | 10 +++-------
 drivers/hv/hyperv_vmbus.h |  2 --
 2 files changed, 3 insertions(+), 9 deletions(-)

Reviewed-by Michael Kelley <[email protected]>