FAILED: patch "[PATCH] Drivers: hv: kvp: Fix the recent regression caused by" failed to apply to 4.19-stable tree

PATCH [email protected] 2 participants 3 messages Nov 26, 2018 1 message not in the archive

Subsystems (2)

Stable branch Hyper-V/Azure core and drivers

lore

Share & tools

lore

[email protected]

author maintainer

2018-11-26 07:40 UTC #0 lore raw

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

Quoted 1 line

From e670de54c813b5bc3672dd1c67871dc60e9206f4 Mon Sep 17 00:00:00 2001

From: Dexuan Cui <[email protected]>
Date: Thu, 18 Oct 2018 05:09:30 +0000
Subject: [PATCH] Drivers: hv: kvp: Fix the recent regression caused by
incorrect clean-up

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 Stephen Hemminger <[email protected]>

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

Cc <[email protected]>

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

Signed-off-by Greg Kroah-Hartman <[email protected]>

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:

Sasha Levin

maintainer

2018-11-26 07:52 UTC #1 lore raw

Quoted 34 lines · On Mon, Nov 26, 2018 at 08:40:09AM +0100, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From e670de54c813b5bc3672dd1c67871dc60e9206f4 Mon Sep 17 00:00:00 2001
From: Dexuan Cui <[email protected]>
Date: Thu, 18 Oct 2018 05:09:30 +0000
Subject: [PATCH] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up
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 Stephen Hemminger <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

Hi Greg,

I think that your scripts need a little tweak to ignore stable tagged commits where the commit they fix isn't in any of the stable trees. For example, this commit fixes a bug that was introduced in 4.20 so it doesn't actually apply to any of the stable trees even though it was tagged for stable.

You can argue that it shouldn't have been tagged for stable to begin with, but I think that we should encourage stable tags with corresponding "Fixes:" tags since that since authors and maintainers don't necessarily know when a patch will be merged, and it's possible that this patch would have been merged in the next release, thus making it stable material.

--
Thanks,
Sasha

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sasha Levin

maintainer

2018-11-26 08:16 UTC #3 lore raw

Quoted 53 lines · Greg KH

Quoted 44 lines · Sasha Levin
Quoted 34 lines · On Mon, Nov 26, 2018 at 08:40:09AM +0100, [email protected] wrote:
The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From e670de54c813b5bc3672dd1c67871dc60e9206f4 Mon Sep 17 00:00:00 2001
From: Dexuan Cui <[email protected]>
Date: Thu, 18 Oct 2018 05:09:30 +0000
Subject: [PATCH] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up
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 Stephen Hemminger <[email protected]>
Signed-off-by Haiyang Zhang <[email protected]>
Cc <[email protected]>
Signed-off-by K. Y. Srinivasan <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 7 lines
Hi Greg,
I think that your scripts need a little tweak to ignore stable tagged commits where the commit they fix isn't in any of the stable trees. For example, this commit fixes a bug that was introduced in 4.20 so it doesn't actually apply to any of the stable trees even though it was tagged for stable.

Quoted 3 lines

Yeah, I saw that this was trying to fix a 4.20-rc patch, but I wanted to let the authors know that this failed and if they had messed up on that tag, they could have resent it.

Fair enough, so maybe a different message here? "No relevant stable trees to apply this patch on"?

Quoted 9 lines

Quoted 6 lines
You can argue that it shouldn't have been tagged for stable to begin with, but I think that we should encourage stable tags with corresponding "Fixes:" tags since that since authors and maintainers don't necessarily know when a patch will be merged, and it's possible that this patch would have been merged in the next release, thus making it stable material.
True, but given when this patch was sent to me, the maintainer should have known better :)

Fair enough, I just want to encourage people "over-tagging" for stable in the general case.

--
Thanks,
Sasha