* [PATCH 0/5] Drivers: hv: Miscellaneous fixes
@ 2018-10-17 3:12 kys
2018-10-17 3:14 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context kys
0 siblings, 1 reply; 21+ messages in thread
From: kys @ 2018-10-17 3:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin,
Michael.H.Kelley, vkuznets
Cc: K. Y. Srinivasan
From: "K. Y. Srinivasan" <kys@microsoft.com>
Miscellaneous fixes.
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 | 2 +-
5 files changed, 28 insertions(+), 16 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context 2018-10-17 3:12 [PATCH 0/5] Drivers: hv: Miscellaneous fixes kys @ 2018-10-17 3:14 ` kys 2018-10-17 3:14 ` [PATCH 2/5] hv_utils: update name in struct hv_driver util_drv kys ` (4 more replies) 0 siblings, 5 replies; 21+ messages in thread From: kys @ 2018-10-17 3:14 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: K. Y. Srinivasan From: "K. Y. Srinivasan" <kys@microsoft.com> 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. Reply-To: kys@microsoft.com Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> --- drivers/hv/hv.c | 10 +++------- drivers/hv/hyperv_vmbus.h | 2 -- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c index 332d7c34be5c..166c2501de17 100644 --- a/drivers/hv/hv.c +++ b/drivers/hv/hv.c @@ -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); diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h index 72eaba3d50fc..f17c06a5e74b 100644 --- a/drivers/hv/hyperv_vmbus.h +++ b/drivers/hv/hyperv_vmbus.h @@ -230,8 +230,6 @@ struct hv_context { void *tsc_page; - bool synic_initialized; - struct hv_per_cpu_context __percpu *cpu_context; /* -- 2.18.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/5] hv_utils: update name in struct hv_driver util_drv 2018-10-17 3:14 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context kys @ 2018-10-17 3:14 ` kys 2018-10-17 3:14 ` [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up kys ` (3 subsequent siblings) 4 siblings, 0 replies; 21+ messages in thread From: kys @ 2018-10-17 3:14 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: Haiyang Zhang, K . Y . Srinivasan From: Haiyang Zhang <haiyangz@microsoft.com> The correct module name is hv_utils. This patch corrects the name in struct hv_driver util_drv. Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> --- drivers/hv/hv_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index 423205077bf6..f10eeb120c8b 100644 --- a/drivers/hv/hv_util.c +++ b/drivers/hv/hv_util.c @@ -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, -- 2.18.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 3:14 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context kys 2018-10-17 3:14 ` [PATCH 2/5] hv_utils: update name in struct hv_driver util_drv kys @ 2018-10-17 3:14 ` kys 2018-10-17 5:07 ` Greg KH 2018-10-17 3:14 ` [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 kys ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: kys @ 2018-10-17 3:14 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: Dexuan Cui, K . Y . Srinivasan, Haiyang Zhang, Stable From: Dexuan Cui <decui@microsoft.com> 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 <decui@microsoft.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: <Stable@vger.kernel.org> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> --- drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index a7513a8a8e37..9fbb15c62c6c 100644 --- a/drivers/hv/hv_kvp.c +++ b/drivers/hv/hv_kvp.c @@ -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; + __attribute__ ((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: -- 2.18.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 3:14 ` [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up kys @ 2018-10-17 5:07 ` Greg KH 2018-10-17 5:11 ` Gustavo A. R. Silva ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Greg KH @ 2018-10-17 5:07 UTC (permalink / raw) To: kys Cc: linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets, Haiyang Zhang, Stable On Wed, Oct 17, 2018 at 03:14:04AM +0000, kys@linuxonhyperv.com wrote: > From: Dexuan Cui <decui@microsoft.com> > > 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 <decui@microsoft.com> > Cc: K. Y. Srinivasan <kys@microsoft.com> > Cc: Haiyang Zhang <haiyangz@microsoft.com> > Cc: Stephen Hemminger <sthemmin@microsoft.com> > Cc: <Stable@vger.kernel.org> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > --- > drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c > index a7513a8a8e37..9fbb15c62c6c 100644 > --- a/drivers/hv/hv_kvp.c > +++ b/drivers/hv/hv_kvp.c > @@ -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; > > + __attribute__ ((fallthrough)); The comment should be sufficient for this, right? I haven't seen many uses of this attribute before, how common is it? > + > + 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; Worst indentation ever :( Yeah, I know it follows the others above it, but you should reconsider it in the future... thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 5:07 ` Greg KH @ 2018-10-17 5:11 ` Gustavo A. R. Silva 2018-10-17 6:02 ` KY Srinivasan ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Gustavo A. R. Silva @ 2018-10-17 5:11 UTC (permalink / raw) To: Greg KH, kys Cc: linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets, Haiyang Zhang, Stable On 10/17/18 7:07 AM, Greg KH wrote: > On Wed, Oct 17, 2018 at 03:14:04AM +0000, kys@linuxonhyperv.com wrote: >> From: Dexuan Cui <decui@microsoft.com> >> >> 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 <decui@microsoft.com> >> Cc: K. Y. Srinivasan <kys@microsoft.com> >> Cc: Haiyang Zhang <haiyangz@microsoft.com> >> Cc: Stephen Hemminger <sthemmin@microsoft.com> >> Cc: <Stable@vger.kernel.org> >> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> >> --- >> drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c >> index a7513a8a8e37..9fbb15c62c6c 100644 >> --- a/drivers/hv/hv_kvp.c >> +++ b/drivers/hv/hv_kvp.c >> @@ -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; >> >> + __attribute__ ((fallthrough)); > > The comment should be sufficient for this, right? I haven't seen many > uses of this attribute before, how common is it? > It's not common at all. Dexuan, please use a comment instead: /* fall through */ Thanks -- Gustavo > >> + >> + 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; > > Worst indentation ever :( > > Yeah, I know it follows the others above it, but you should reconsider > it in the future... > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 5:07 ` Greg KH 2018-10-17 5:11 ` Gustavo A. R. Silva @ 2018-10-17 6:02 ` KY Srinivasan 2018-10-17 17:56 ` Dexuan Cui 2018-10-17 6:22 ` Dan Carpenter 2018-10-17 18:01 ` Dexuan Cui 3 siblings, 1 reply; 21+ messages in thread From: KY Srinivasan @ 2018-10-17 6:02 UTC (permalink / raw) To: Greg KH Cc: linux-kernel, devel, olaf, apw, jasowang, Stephen Hemminger, Michael Kelley (EOSG), vkuznets, Haiyang Zhang, Stable > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, October 16, 2018 10:07 PM > To: KY Srinivasan <kys@microsoft.com> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; Stephen > Hemminger <sthemmin@microsoft.com>; Michael Kelley (EOSG) > <Michael.H.Kelley@microsoft.com>; vkuznets <vkuznets@redhat.com>; > Haiyang Zhang <haiyangz@microsoft.com>; Stable@vger.kernel.org > Subject: Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by > incorrect clean-up > > On Wed, Oct 17, 2018 at 03:14:04AM +0000, kys@linuxonhyperv.com wrote: > > From: Dexuan Cui <decui@microsoft.com> > > > > 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 <decui@microsoft.com> > > Cc: K. Y. Srinivasan <kys@microsoft.com> > > Cc: Haiyang Zhang <haiyangz@microsoft.com> > > Cc: Stephen Hemminger <sthemmin@microsoft.com> > > Cc: <Stable@vger.kernel.org> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > > --- > > drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c > > index a7513a8a8e37..9fbb15c62c6c 100644 > > --- a/drivers/hv/hv_kvp.c > > +++ b/drivers/hv/hv_kvp.c > > @@ -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; > > > > + __attribute__ ((fallthrough)); > > The comment should be sufficient for this, right? I haven't seen many > uses of this attribute before, how common is it? Yes; a common should be sufficient. > > > > + > > + 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; > > Worst indentation ever :( > > Yeah, I know it follows the others above it, but you should reconsider > it in the future... > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 6:02 ` KY Srinivasan @ 2018-10-17 17:56 ` Dexuan Cui 0 siblings, 0 replies; 21+ messages in thread From: Dexuan Cui @ 2018-10-17 17:56 UTC (permalink / raw) To: KY Srinivasan, Greg KH Cc: olaf, Stephen Hemminger, jasowang, linux-kernel, Stable, Michael Kelley, apw, devel, vkuznets, Haiyang Zhang, Gustavo A. R. Silva, Dan Carpenter > From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf Of > KY Srinivasan > Sent: Tuesday, October 16, 2018 23:02 > > > --- a/drivers/hv/hv_kvp.c > > > +++ b/drivers/hv/hv_kvp.c > > > @@ -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; > > > > > > + __attribute__ ((fallthrough)); > > > > The comment should be sufficient for this, right? I haven't seen many > > uses of this attribute before, how common is it? > > Yes; a common should be sufficient. You all are right. Right now, I realized the gcc warning can also be suppressed by a simple line of comment: /* fallthrough */ It looks gcc treats this comment specially. If I add something more in the comment, like /* add fallthrough */ , the warning can not be suppressed. :-) I'll do a new version for KY. Thank you all! -- Dexuan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 5:07 ` Greg KH 2018-10-17 5:11 ` Gustavo A. R. Silva 2018-10-17 6:02 ` KY Srinivasan @ 2018-10-17 6:22 ` Dan Carpenter 2018-10-20 14:42 ` Miguel Ojeda 2018-10-17 18:01 ` Dexuan Cui 3 siblings, 1 reply; 21+ messages in thread From: Dan Carpenter @ 2018-10-17 6:22 UTC (permalink / raw) To: Greg KH Cc: kys, olaf, sthemmin, jasowang, linux-kernel, Stable, Michael.H.Kelley, apw, devel, vkuznets, Haiyang Zhang On Wed, Oct 17, 2018 at 07:07:05AM +0200, Greg KH wrote: > On Wed, Oct 17, 2018 at 03:14:04AM +0000, kys@linuxonhyperv.com wrote: > > From: Dexuan Cui <decui@microsoft.com> > > > > 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 <decui@microsoft.com> > > Cc: K. Y. Srinivasan <kys@microsoft.com> > > Cc: Haiyang Zhang <haiyangz@microsoft.com> > > Cc: Stephen Hemminger <sthemmin@microsoft.com> > > Cc: <Stable@vger.kernel.org> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > > --- > > drivers/hv/hv_kvp.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c > > index a7513a8a8e37..9fbb15c62c6c 100644 > > --- a/drivers/hv/hv_kvp.c > > +++ b/drivers/hv/hv_kvp.c > > @@ -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; > > > > + __attribute__ ((fallthrough)); > > The comment should be sufficient for this, right? I haven't seen many > uses of this attribute before, how common is it? > It's not common at all. It should be wrapped in a macro and put into compiler.h. But I hope it does become adopted. It's better than randomly grepping for non-standard comments. regards, dan carpenter ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 6:22 ` Dan Carpenter @ 2018-10-20 14:42 ` Miguel Ojeda 2018-10-20 19:22 ` Dan Carpenter 0 siblings, 1 reply; 21+ messages in thread From: Miguel Ojeda @ 2018-10-20 14:42 UTC (permalink / raw) To: Dan Cc: Greg KH, kys, olaf, sthemmin, jasowang, linux-kernel, Stable, Michael.H.Kelley, Robo Bot, devel, vkuznets, haiyangz +On Wed, Oct 17, 2018 at 8:25 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > It's not common at all. It should be wrapped in a macro and put into > compiler.h. > > But I hope it does become adopted. It's better than randomly grepping > for non-standard comments. Using an attribute is indeed better whenever possible. In C++17 it is an standard attribute and there have been proposals to include some of them for C as well since 2016 at least, e.g. the latest for fallthrough at: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf I have taken a quick look into supporting it (typing it here to save it on the mailing list :-), and we have: * gcc >= 7.1 supports -Wimplicit-fallthrough with __attribute__((fallthrough)), the comment syntax and the C++ [[fallthrough]] syntax. * gcc < 7.1 complains about empty declarations (it does not know about attributes for null statements) and also -Wdeclaration-after-statement. * clang 7 supports -Wimplicit-fallthrough (not enabled in -Wall/extra/pedantic like gcc, though) but *only* in C++ mode and with the C++ syntax [[fallthrough]]. In other words, in C mode, no syntax works and no diagnostics are emitted. It complains about Wmissing-declarations. [IMO they should allow the __attribute__ syntax for fallthrough (and enable it on C mode) to be compatible with gcc. Maybe they are simply waiting for the C2x attributes... :-)] * icc 19 does not know about -Wimplicit-fallthrough at all (but seems to allow [[fallthrough]] on C++17 mode to comply with the standard). Therefore, the only improvement we could do right now is starting to use the attribute for gcc > 7.1, and a comment for everybody else. However, even if that was worth the trouble of changing the 2500+ instances of fall through markings that we have, comments are replaced before the preprocessor stage, so we would need some (probably non-portable) macro magic. So, I would say, let's revisit this again in a few years. Possibly, when we move the minimum version to gcc 7.1, clang and icc may both support the fallthrough warning in C mode; so that we can always use the __attribute__((fallthrough)) unconditionally under a __fallthrough #define. Cheers, Miguel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-20 14:42 ` Miguel Ojeda @ 2018-10-20 19:22 ` Dan Carpenter 2018-10-21 4:15 ` Miguel Ojeda 0 siblings, 1 reply; 21+ messages in thread From: Dan Carpenter @ 2018-10-20 19:22 UTC (permalink / raw) To: Miguel Ojeda Cc: olaf, sthemmin, Greg KH, jasowang, linux-kernel, Stable, Michael.H.Kelley, Robo Bot, devel, vkuznets, haiyangz On Sat, Oct 20, 2018 at 04:42:07PM +0200, Miguel Ojeda wrote: > +On Wed, Oct 17, 2018 at 8:25 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > It's not common at all. It should be wrapped in a macro and put into > > compiler.h. > > > > But I hope it does become adopted. It's better than randomly grepping > > for non-standard comments. > > Using an attribute is indeed better whenever possible. In C++17 it is > an standard attribute and there have been proposals to include some of > them for C as well since 2016 at least, e.g. the latest for > fallthrough at: > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf > > I have taken a quick look into supporting it (typing it here to save > it on the mailing list :-), and we have: > > * gcc >= 7.1 supports -Wimplicit-fallthrough with > __attribute__((fallthrough)), the comment syntax and the C++ > [[fallthrough]] syntax. > * gcc < 7.1 complains about empty declarations (it does not know > about attributes for null statements) and also > -Wdeclaration-after-statement. I'm not sure I understand about empty decalarations. The idea is that we would do: case 3: frob(); __fall_through(); case 4: frob(); #if GCC > 7.1 #define __fall_through() __attribute__((fallthrough)) #else #define __fall_through() #endif So long as GCC and static analysis tools understand about the attribute that's enought to catch the bugs. No one cares what clang and icc do. We would just disable the fall through warnings on those until they are updated to support the fallthrough attribute. We wouldn't update all the fall through comments until later, but going forward people could use the __fall_through() macro if they want. regards, dan carpenter ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-20 19:22 ` Dan Carpenter @ 2018-10-21 4:15 ` Miguel Ojeda 0 siblings, 0 replies; 21+ messages in thread From: Miguel Ojeda @ 2018-10-21 4:15 UTC (permalink / raw) To: Dan Cc: olaf, sthemmin, Greg KH, jasowang, linux-kernel, Stable, Michael.H.Kelley, Robo Bot, devel, vkuznets, haiyangz Hi Dan, On Sat, Oct 20, 2018 at 9:22 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Sat, Oct 20, 2018 at 04:42:07PM +0200, Miguel Ojeda wrote: > > Using an attribute is indeed better whenever possible. In C++17 it is > > an standard attribute and there have been proposals to include some of > > them for C as well since 2016 at least, e.g. the latest for > > fallthrough at: > > > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf > > > > I have taken a quick look into supporting it (typing it here to save > > it on the mailing list :-), and we have: > > > > * gcc >= 7.1 supports -Wimplicit-fallthrough with > > __attribute__((fallthrough)), the comment syntax and the C++ > > [[fallthrough]] syntax. > > * gcc < 7.1 complains about empty declarations (it does not know > > about attributes for null statements) and also > > -Wdeclaration-after-statement. > > I'm not sure I understand about empty decalarations. The idea is that > we would do: That paragraph tried to explain that gcc < 7.1 did not know about __attribute__((fallthrough)); i.e. that everything was introduced in gcc 7.1. Anyway, the conclusion was a neuron misfiring of mine -- in my mind I was thinking clang supported the comment syntax (when I clearly wrote that it didn't). Never mind --- thanks for pointing it out! > > case 3: > frob(); > __fall_through(); > case 4: > frob(); > > #if GCC > 7.1 > #define __fall_through() __attribute__((fallthrough)) > #else > #define __fall_through() > #endif Yes, of course, that is what we do for other attributes -- actually in -next we have pending a better way for checking, using __has_attribute: #if __has_attribute(fallthrough) #define __fallthrough __attribute__((fallthrough)) #else #define __fallthrough #endif > > So long as GCC and static analysis tools understand about the attribute > that's enought to catch the bugs. No one cares what clang and icc do. Not so sure about that -- there are quite some people looking forward to building Linux with clang, even if only to have another compiler to check for warnings and to use the clang/llvm-related tools (and to write new ones). > We would just disable the fall through warnings on those until they are > updated to support the fallthrough attribute. You mean if they start supporting the warning but not the attribute? I don't think that would be likely (i.e. if clang enables the warning on C mode, they will have to introduce a way to suppress it; which should be the attribute (at least), since they typically like to be compatible with gcc and since they already support it in C++ >= 11), but everything is possible. > > We wouldn't update all the fall through comments until later, but going > forward people could use the __fall_through() macro if they want. Agreed. I will introduce it in the compiler-attributes tree -- should be there for -rc1 if no one complains. CC'ing all of you in the patch. Cheers, Miguel ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up 2018-10-17 5:07 ` Greg KH ` (2 preceding siblings ...) 2018-10-17 6:22 ` Dan Carpenter @ 2018-10-17 18:01 ` Dexuan Cui 3 siblings, 0 replies; 21+ messages in thread From: Dexuan Cui @ 2018-10-17 18:01 UTC (permalink / raw) To: Greg KH, KY Srinivasan Cc: olaf, Stephen Hemminger, jasowang, linux-kernel, Stable, Michael Kelley, apw, devel, vkuznets, Haiyang Zhang > From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf Of > Greg KH > Sent: Tuesday, October 16, 2018 22:07 > > ... > > + 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; > > Worst indentation ever :( > > Yeah, I know it follows the others above it, but you should reconsider > it in the future... > greg k-h Agreed. We should consider improving this in future. -- Dexuan ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 2018-10-17 3:14 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context kys 2018-10-17 3:14 ` [PATCH 2/5] hv_utils: update name in struct hv_driver util_drv kys 2018-10-17 3:14 ` [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up kys @ 2018-10-17 3:14 ` kys 2018-10-17 5:04 ` Greg KH 2018-10-17 3:14 ` [PATCH 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1 kys 2018-10-17 5:04 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context Greg KH 4 siblings, 1 reply; 21+ messages in thread From: kys @ 2018-10-17 3:14 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: Dexuan Cui, K . Y . Srinivasan, Haiyang Zhang, Stable From: Dexuan Cui <decui@microsoft.com> 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 <decui@microsoft.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: <Stable@vger.kernel.org> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> --- drivers/hv/hv_kvp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index 9fbb15c62c6c..3b8590ff94ba 100644 --- a/drivers/hv/hv_kvp.c +++ b/drivers/hv/hv_kvp.c @@ -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: -- 2.18.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 2018-10-17 3:14 ` [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 kys @ 2018-10-17 5:04 ` Greg KH 2018-10-17 5:59 ` KY Srinivasan 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2018-10-17 5:04 UTC (permalink / raw) To: kys Cc: linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets, Haiyang Zhang, Stable On Wed, Oct 17, 2018 at 03:14:05AM +0000, kys@linuxonhyperv.com wrote: > From: Dexuan Cui <decui@microsoft.com> > > 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 <decui@microsoft.com> > Cc: K. Y. Srinivasan <kys@microsoft.com> > Cc: Haiyang Zhang <haiyangz@microsoft.com> > Cc: Stephen Hemminger <sthemmin@microsoft.com> > Cc: <Stable@vger.kernel.org> Why is this a stable patch? confused, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 2018-10-17 5:04 ` Greg KH @ 2018-10-17 5:59 ` KY Srinivasan 0 siblings, 0 replies; 21+ messages in thread From: KY Srinivasan @ 2018-10-17 5:59 UTC (permalink / raw) To: Greg KH Cc: linux-kernel, devel, olaf, apw, jasowang, Stephen Hemminger, Michael Kelley (EOSG), vkuznets, Haiyang Zhang, Stable > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, October 16, 2018 10:04 PM > To: KY Srinivasan <kys@microsoft.com> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; Stephen > Hemminger <sthemmin@microsoft.com>; Michael Kelley (EOSG) > <Michael.H.Kelley@microsoft.com>; vkuznets <vkuznets@redhat.com>; > Haiyang Zhang <haiyangz@microsoft.com>; Stable@vger.kernel.org > Subject: Re: [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 > > On Wed, Oct 17, 2018 at 03:14:05AM +0000, kys@linuxonhyperv.com wrote: > > From: Dexuan Cui <decui@microsoft.com> > > > > 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 <decui@microsoft.com> > > Cc: K. Y. Srinivasan <kys@microsoft.com> > > Cc: Haiyang Zhang <haiyangz@microsoft.com> > > Cc: Stephen Hemminger <sthemmin@microsoft.com> > > Cc: <Stable@vger.kernel.org> > > Why is this a stable patch? My mistake; I will resend. K. Y > > confused, > > greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1 2018-10-17 3:14 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context kys ` (2 preceding siblings ...) 2018-10-17 3:14 ` [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 kys @ 2018-10-17 3:14 ` kys 2018-10-17 5:07 ` Greg KH 2018-10-17 5:04 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context Greg KH 4 siblings, 1 reply; 21+ messages in thread From: kys @ 2018-10-17 3:14 UTC (permalink / raw) To: gregkh, linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets Cc: Dexuan Cui, K . Y . Srinivasan, Haiyang Zhang, Stable From: Dexuan Cui <decui@microsoft.com> 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 Signed-off-by: Dexuan Cui <decui@microsoft.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: <Stable@vger.kernel.org> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> --- tools/hv/hv_kvp_daemon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c index bbb2a8ef367c..b7c2cf7eaba5 100644 --- a/tools/hv/hv_kvp_daemon.c +++ b/tools/hv/hv_kvp_daemon.c @@ -1176,7 +1176,7 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val) int error = 0; char if_file[PATH_MAX]; FILE *file; - char cmd[PATH_MAX]; + char cmd[PATH_MAX * 2]; char *mac_addr; /* -- 2.18.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1 2018-10-17 3:14 ` [PATCH 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1 kys @ 2018-10-17 5:07 ` Greg KH 2018-10-17 19:57 ` Dexuan Cui 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2018-10-17 5:07 UTC (permalink / raw) To: kys Cc: linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets, Haiyang Zhang, Stable On Wed, Oct 17, 2018 at 03:14:06AM +0000, kys@linuxonhyperv.com wrote: > From: Dexuan Cui <decui@microsoft.com> > > 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 > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > Cc: K. Y. Srinivasan <kys@microsoft.com> > Cc: Haiyang Zhang <haiyangz@microsoft.com> > Cc: Stephen Hemminger <sthemmin@microsoft.com> > Cc: <Stable@vger.kernel.org> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com> > --- > tools/hv/hv_kvp_daemon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c > index bbb2a8ef367c..b7c2cf7eaba5 100644 > --- a/tools/hv/hv_kvp_daemon.c > +++ b/tools/hv/hv_kvp_daemon.c > @@ -1176,7 +1176,7 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val) > int error = 0; > char if_file[PATH_MAX]; > FILE *file; > - char cmd[PATH_MAX]; > + char cmd[PATH_MAX * 2]; Overkill? ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1 2018-10-17 5:07 ` Greg KH @ 2018-10-17 19:57 ` Dexuan Cui 0 siblings, 0 replies; 21+ messages in thread From: Dexuan Cui @ 2018-10-17 19:57 UTC (permalink / raw) To: Greg KH, KY Srinivasan Cc: olaf, Stephen Hemminger, jasowang, linux-kernel, Stable, Michael Kelley, apw, devel, vkuznets, Haiyang Zhang > From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf Of > Greg KH > Sent: Tuesday, October 16, 2018 22:07 > > On Wed, Oct 17, 2018 at 03:14:06AM +0000, kys@linuxonhyperv.com wrote: > > From: Dexuan Cui <decui@microsoft.com> > > > > 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 > > ... > > --- a/tools/hv/hv_kvp_daemon.c > > +++ b/tools/hv/hv_kvp_daemon.c > > @@ -1176,7 +1176,7 @@ static int kvp_set_ip_info(char *if_name, struct > hv_kvp_ipaddr_value *new_val) > > int error = 0; > > char if_file[PATH_MAX]; > > FILE *file; > > - char cmd[PATH_MAX]; > > + char cmd[PATH_MAX * 2]; > > Overkill? Well, I found the -Wformat-truncation warning can be suppressed by checking the return value of snprintf, so I'm going to do a new patch for KY to resend like this: @@ -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,19 @@ 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; + } Thanks, -- Dexuan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context 2018-10-17 3:14 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context kys ` (3 preceding siblings ...) 2018-10-17 3:14 ` [PATCH 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1 kys @ 2018-10-17 5:04 ` Greg KH 2018-10-17 5:59 ` KY Srinivasan 4 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2018-10-17 5:04 UTC (permalink / raw) To: kys Cc: linux-kernel, devel, olaf, apw, jasowang, sthemmin, Michael.H.Kelley, vkuznets On Wed, Oct 17, 2018 at 03:14:02AM +0000, kys@linuxonhyperv.com wrote: > From: "K. Y. Srinivasan" <kys@microsoft.com> > > 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. > > Reply-To: kys@microsoft.com Why is this here? greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context 2018-10-17 5:04 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context Greg KH @ 2018-10-17 5:59 ` KY Srinivasan 0 siblings, 0 replies; 21+ messages in thread From: KY Srinivasan @ 2018-10-17 5:59 UTC (permalink / raw) To: Greg KH Cc: linux-kernel, devel, olaf, apw, jasowang, Stephen Hemminger, Michael Kelley (EOSG), vkuznets > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, October 16, 2018 10:05 PM > To: KY Srinivasan <kys@microsoft.com> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; Stephen > Hemminger <sthemmin@microsoft.com>; Michael Kelley (EOSG) > <Michael.H.Kelley@microsoft.com>; vkuznets <vkuznets@redhat.com> > Subject: Re: [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in > hv_context > > On Wed, Oct 17, 2018 at 03:14:02AM +0000, kys@linuxonhyperv.com wrote: > > From: "K. Y. Srinivasan" <kys@microsoft.com> > > > > 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. > > > > Reply-To: kys@microsoft.com > > Why is this here? No sure how this happened; will fix it and resend. K. Y > > greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-10-21 4:16 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-17 3:12 [PATCH 0/5] Drivers: hv: Miscellaneous fixes kys 2018-10-17 3:14 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context kys 2018-10-17 3:14 ` [PATCH 2/5] hv_utils: update name in struct hv_driver util_drv kys 2018-10-17 3:14 ` [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up kys 2018-10-17 5:07 ` Greg KH 2018-10-17 5:11 ` Gustavo A. R. Silva 2018-10-17 6:02 ` KY Srinivasan 2018-10-17 17:56 ` Dexuan Cui 2018-10-17 6:22 ` Dan Carpenter 2018-10-20 14:42 ` Miguel Ojeda 2018-10-20 19:22 ` Dan Carpenter 2018-10-21 4:15 ` Miguel Ojeda 2018-10-17 18:01 ` Dexuan Cui 2018-10-17 3:14 ` [PATCH 4/5] Drivers: hv: kvp: Use %u to print U32 kys 2018-10-17 5:04 ` Greg KH 2018-10-17 5:59 ` KY Srinivasan 2018-10-17 3:14 ` [PATCH 5/5] Tools: hv: kvp: Fix a warning of buffer overflow with gcc 8.0.1 kys 2018-10-17 5:07 ` Greg KH 2018-10-17 19:57 ` Dexuan Cui 2018-10-17 5:04 ` [PATCH 1/5] Drivers: hv: vmbus: Get rid of unnecessary state in hv_context Greg KH 2018-10-17 5:59 ` KY Srinivasan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).