RE: [PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't

2017-03-04 Thread KY Srinivasan 


> -Original Message-
> From: k...@exchange.microsoft.com [mailto:k...@exchange.microsoft.com]
> Sent: Monday, February 27, 2017 3:18 PM
> To: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> vkuzn...@redhat.com; jasow...@redhat.com;
> leann.ogasaw...@canonical.com
> Cc: Dexuan Cui ; KY Srinivasan
> ; Haiyang Zhang ; Stephen
> Hemminger 
> Subject: [PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release 
> event,
> if we shouldn't
> 
> Thissenderfailedourfrauddetection
> p;checksandmaynotbewhothey&
> nbsp;appeartobe.Learnabout href="http://aka.ms/LearnAboutSpoofing;>spoofing
> 
> From: Dexuan Cui 
> 
> If the daemon is NOT running at all, when we disable the util device from
> Hyper-V Manager (or sometimes the host can rescind a util device and then
> re-offer it), we'll hang in util_remove -> hv_kvp_deinit ->
> wait_for_completion(_event), because this code path doesn't run:
> hvt_op_release -> ... -> kvp_on_reset -> complete(_event).
> 
> Due to this, we even can't reboot the VM properly.
> 
> The patch tracks if the dev file is opened or not, and we only need to
> wait if it's opened.
> 
> Fixes: 5a66fecbf6aa ("Drivers: hv: util: kvp: Fix a rescind processing issue")
> 
> Signed-off-by: Dexuan Cui 
> Cc: Vitaly Kuznetsov 
> Cc: "K. Y. Srinivasan" 
> Cc: Haiyang Zhang 
> Cc: Stephen Hemminger 
> Signed-off-by: K. Y. Srinivasan 
> ---
>  drivers/hv/hv_fcopy.c   |5 -
>  drivers/hv/hv_kvp.c |6 +-
>  drivers/hv/hv_snapshot.c|5 -
>  drivers/hv/hv_utils_transport.c |2 ++
>  drivers/hv/hv_utils_transport.h |1 +
>  5 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index 9aee601..545cf43 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -358,8 +358,11 @@ int hv_fcopy_init(struct hv_util_service *srv)
> 
>  void hv_fcopy_deinit(void)
>  {
> +   bool wait = hvt->dev_opened;
> +
> fcopy_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(_timeout_work);
> hvutil_transport_destroy(hvt);
> -   wait_for_completion(_event);
> +   if (wait)
> +   wait_for_completion(_event);
>  }
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index de26371..15c7873 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -742,10 +742,14 @@ static void kvp_on_reset(void)
> 
>  void hv_kvp_deinit(void)
>  {
> +   bool wait = hvt->dev_opened;
> +
> kvp_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(_host_handshake_work);
> cancel_delayed_work_sync(_timeout_work);
> cancel_work_sync(_sendkey_work);
> hvutil_transport_destroy(hvt);
> -   wait_for_completion(_event);
> +
> +   if (wait)
> +   wait_for_completion(_event);
>  }
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index bcc03f0..3847f19 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -396,9 +396,12 @@ static void vss_on_reset(void)
> 
>  void hv_vss_deinit(void)
>  {
> +   bool wait = hvt->dev_opened;
> +
> vss_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(_timeout_work);
> cancel_work_sync(_handle_request_work);
> hvutil_transport_destroy(hvt);
> -   wait_for_completion(_event);
> +   if (wait)
> +   wait_for_completion(_event);
>  }
> diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
> index c235a95..05e0648 100644
> --- a/drivers/hv/hv_utils_transport.c
> +++ b/drivers/hv/hv_utils_transport.c
> @@ -153,6 +153,7 @@ static int hvt_op_open(struct inode *inode, struct file
> *file)
> 
> if (issue_reset)
> hvt_reset(hvt);
> +   hvt->dev_opened = (hvt->mode == HVUTIL_TRANSPORT_CHARDEV)
> && !ret;
> 
> mutex_unlock(>lock);
> 
> @@ -182,6 +183,7 @@ static int hvt_op_release(struct inode *inode, struct
> file *file)
>  * connects back.
>  */
> hvt_reset(hvt);
> +   hvt->dev_opened = false;
> mutex_unlock(>lock);
> 
> if (mode_old == HVUTIL_TRANSPORT_DESTROY)
> diff --git a/drivers/hv/hv_utils_transport.h b/drivers/hv/hv_utils_transport.h
> index d98f522..9871283 100644
> --- a/drivers/hv/hv_utils_transport.h
> +++ b/drivers/hv/hv_utils_transport.h
> @@ -32,6 +32,7 @@ struct hvutil_transport {
> int mode;   /* hvutil_transport_mode */
> struct file_operations fops;/* file operations */
> struct miscdevice mdev; /* misc device */
> +   bool

RE: [PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't

2017-03-04 Thread KY Srinivasan 


> -Original Message-
> From: k...@exchange.microsoft.com [mailto:k...@exchange.microsoft.com]
> Sent: Monday, February 27, 2017 3:18 PM
> To: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> vkuzn...@redhat.com; jasow...@redhat.com;
> leann.ogasaw...@canonical.com
> Cc: Dexuan Cui ; KY Srinivasan
> ; Haiyang Zhang ; Stephen
> Hemminger 
> Subject: [PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release 
> event,
> if we shouldn't
> 
> Thissenderfailedourfrauddetection
> p;checksandmaynotbewhothey&
> nbsp;appeartobe.Learnabout href="http://aka.ms/LearnAboutSpoofing;>spoofing
> 
> From: Dexuan Cui 
> 
> If the daemon is NOT running at all, when we disable the util device from
> Hyper-V Manager (or sometimes the host can rescind a util device and then
> re-offer it), we'll hang in util_remove -> hv_kvp_deinit ->
> wait_for_completion(_event), because this code path doesn't run:
> hvt_op_release -> ... -> kvp_on_reset -> complete(_event).
> 
> Due to this, we even can't reboot the VM properly.
> 
> The patch tracks if the dev file is opened or not, and we only need to
> wait if it's opened.
> 
> Fixes: 5a66fecbf6aa ("Drivers: hv: util: kvp: Fix a rescind processing issue")
> 
> Signed-off-by: Dexuan Cui 
> Cc: Vitaly Kuznetsov 
> Cc: "K. Y. Srinivasan" 
> Cc: Haiyang Zhang 
> Cc: Stephen Hemminger 
> Signed-off-by: K. Y. Srinivasan 
> ---
>  drivers/hv/hv_fcopy.c   |5 -
>  drivers/hv/hv_kvp.c |6 +-
>  drivers/hv/hv_snapshot.c|5 -
>  drivers/hv/hv_utils_transport.c |2 ++
>  drivers/hv/hv_utils_transport.h |1 +
>  5 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index 9aee601..545cf43 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -358,8 +358,11 @@ int hv_fcopy_init(struct hv_util_service *srv)
> 
>  void hv_fcopy_deinit(void)
>  {
> +   bool wait = hvt->dev_opened;
> +
> fcopy_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(_timeout_work);
> hvutil_transport_destroy(hvt);
> -   wait_for_completion(_event);
> +   if (wait)
> +   wait_for_completion(_event);
>  }
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index de26371..15c7873 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -742,10 +742,14 @@ static void kvp_on_reset(void)
> 
>  void hv_kvp_deinit(void)
>  {
> +   bool wait = hvt->dev_opened;
> +
> kvp_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(_host_handshake_work);
> cancel_delayed_work_sync(_timeout_work);
> cancel_work_sync(_sendkey_work);
> hvutil_transport_destroy(hvt);
> -   wait_for_completion(_event);
> +
> +   if (wait)
> +   wait_for_completion(_event);
>  }
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index bcc03f0..3847f19 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -396,9 +396,12 @@ static void vss_on_reset(void)
> 
>  void hv_vss_deinit(void)
>  {
> +   bool wait = hvt->dev_opened;
> +
> vss_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(_timeout_work);
> cancel_work_sync(_handle_request_work);
> hvutil_transport_destroy(hvt);
> -   wait_for_completion(_event);
> +   if (wait)
> +   wait_for_completion(_event);
>  }
> diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
> index c235a95..05e0648 100644
> --- a/drivers/hv/hv_utils_transport.c
> +++ b/drivers/hv/hv_utils_transport.c
> @@ -153,6 +153,7 @@ static int hvt_op_open(struct inode *inode, struct file
> *file)
> 
> if (issue_reset)
> hvt_reset(hvt);
> +   hvt->dev_opened = (hvt->mode == HVUTIL_TRANSPORT_CHARDEV)
> && !ret;
> 
> mutex_unlock(>lock);
> 
> @@ -182,6 +183,7 @@ static int hvt_op_release(struct inode *inode, struct
> file *file)
>  * connects back.
>  */
> hvt_reset(hvt);
> +   hvt->dev_opened = false;
> mutex_unlock(>lock);
> 
> if (mode_old == HVUTIL_TRANSPORT_DESTROY)
> diff --git a/drivers/hv/hv_utils_transport.h b/drivers/hv/hv_utils_transport.h
> index d98f522..9871283 100644
> --- a/drivers/hv/hv_utils_transport.h
> +++ b/drivers/hv/hv_utils_transport.h
> @@ -32,6 +32,7 @@ struct hvutil_transport {
> int mode;   /* hvutil_transport_mode */
> struct file_operations fops;/* file operations */
> struct miscdevice mdev; /* misc device */
> +   bool   dev_opened;  /* Is the device opened? */
> struct cb_id cn_id; /* CN_*_IDX/CN_*_VAL */
> struct list_head list;  /* hvt_list */
> int (*on_msg)(void *, int); /*

RE: [PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't

2017-03-01 Thread Dexuan Cui 
> From: k...@exchange.microsoft.com [mailto:k...@exchange.microsoft.com]
> Sent: Tuesday, February 28, 2017 07:18
> To: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> ...
> From: Dexuan Cui 
> 
> If the daemon is NOT running at all, when we disable the util device from
> Hyper-V Manager (or sometimes the host can rescind a util device and then
> re-offer it), we'll hang in util_remove -> hv_kvp_deinit ->
> wait_for_completion(_event), because this code path doesn't run:
> hvt_op_release -> ... -> kvp_on_reset -> complete(_event).
> 
> Due to this, we even can't reboot the VM properly.
> 
> The patch tracks if the dev file is opened or not, and we only need to
> wait if it's opened.
> 
> Fixes: 5a66fecbf6aa ("Drivers: hv: util: kvp: Fix a rescind processing issue")
> 
> Signed-off-by: Dexuan Cui 
> ...

Please ignore this patch, since Vitaly posted a better fix instead.

Thanks,
-- Dexuan

RE: [PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't

2017-03-01 Thread Dexuan Cui 
> From: k...@exchange.microsoft.com [mailto:k...@exchange.microsoft.com]
> Sent: Tuesday, February 28, 2017 07:18
> To: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> ...
> From: Dexuan Cui 
> 
> If the daemon is NOT running at all, when we disable the util device from
> Hyper-V Manager (or sometimes the host can rescind a util device and then
> re-offer it), we'll hang in util_remove -> hv_kvp_deinit ->
> wait_for_completion(_event), because this code path doesn't run:
> hvt_op_release -> ... -> kvp_on_reset -> complete(_event).
> 
> Due to this, we even can't reboot the VM properly.
> 
> The patch tracks if the dev file is opened or not, and we only need to
> wait if it's opened.
> 
> Fixes: 5a66fecbf6aa ("Drivers: hv: util: kvp: Fix a rescind processing issue")
> 
> Signed-off-by: Dexuan Cui 
> ...

Please ignore this patch, since Vitaly posted a better fix instead.

Thanks,
-- Dexuan

[PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't

2017-02-27 Thread kys 
From: Dexuan Cui 

If the daemon is NOT running at all, when we disable the util device from
Hyper-V Manager (or sometimes the host can rescind a util device and then
re-offer it), we'll hang in util_remove -> hv_kvp_deinit ->
wait_for_completion(_event), because this code path doesn't run:
hvt_op_release -> ... -> kvp_on_reset -> complete(_event).

Due to this, we even can't reboot the VM properly.

The patch tracks if the dev file is opened or not, and we only need to
wait if it's opened.

Fixes: 5a66fecbf6aa ("Drivers: hv: util: kvp: Fix a rescind processing issue")

Signed-off-by: Dexuan Cui 
Cc: Vitaly Kuznetsov 
Cc: "K. Y. Srinivasan" 
Cc: Haiyang Zhang 
Cc: Stephen Hemminger 
Signed-off-by: K. Y. Srinivasan 
---
 drivers/hv/hv_fcopy.c   |5 -
 drivers/hv/hv_kvp.c |6 +-
 drivers/hv/hv_snapshot.c|5 -
 drivers/hv/hv_utils_transport.c |2 ++
 drivers/hv/hv_utils_transport.h |1 +
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index 9aee601..545cf43 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -358,8 +358,11 @@ int hv_fcopy_init(struct hv_util_service *srv)
 
 void hv_fcopy_deinit(void)
 {
+   bool wait = hvt->dev_opened;
+
fcopy_transaction.state = HVUTIL_DEVICE_DYING;
cancel_delayed_work_sync(_timeout_work);
hvutil_transport_destroy(hvt);
-   wait_for_completion(_event);
+   if (wait)
+   wait_for_completion(_event);
 }
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index de26371..15c7873 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -742,10 +742,14 @@ static void kvp_on_reset(void)
 
 void hv_kvp_deinit(void)
 {
+   bool wait = hvt->dev_opened;
+
kvp_transaction.state = HVUTIL_DEVICE_DYING;
cancel_delayed_work_sync(_host_handshake_work);
cancel_delayed_work_sync(_timeout_work);
cancel_work_sync(_sendkey_work);
hvutil_transport_destroy(hvt);
-   wait_for_completion(_event);
+
+   if (wait)
+   wait_for_completion(_event);
 }
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index bcc03f0..3847f19 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -396,9 +396,12 @@ static void vss_on_reset(void)
 
 void hv_vss_deinit(void)
 {
+   bool wait = hvt->dev_opened;
+
vss_transaction.state = HVUTIL_DEVICE_DYING;
cancel_delayed_work_sync(_timeout_work);
cancel_work_sync(_handle_request_work);
hvutil_transport_destroy(hvt);
-   wait_for_completion(_event);
+   if (wait)
+   wait_for_completion(_event);
 }
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index c235a95..05e0648 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -153,6 +153,7 @@ static int hvt_op_open(struct inode *inode, struct file 
*file)
 
if (issue_reset)
hvt_reset(hvt);
+   hvt->dev_opened = (hvt->mode == HVUTIL_TRANSPORT_CHARDEV) && !ret;
 
mutex_unlock(>lock);
 
@@ -182,6 +183,7 @@ static int hvt_op_release(struct inode *inode, struct file 
*file)
 * connects back.
 */
hvt_reset(hvt);
+   hvt->dev_opened = false;
mutex_unlock(>lock);
 
if (mode_old == HVUTIL_TRANSPORT_DESTROY)
diff --git a/drivers/hv/hv_utils_transport.h b/drivers/hv/hv_utils_transport.h
index d98f522..9871283 100644
--- a/drivers/hv/hv_utils_transport.h
+++ b/drivers/hv/hv_utils_transport.h
@@ -32,6 +32,7 @@ struct hvutil_transport {
int mode;   /* hvutil_transport_mode */
struct file_operations fops;/* file operations */
struct miscdevice mdev; /* misc device */
+   bool   dev_opened;  /* Is the device opened? */
struct cb_id cn_id; /* CN_*_IDX/CN_*_VAL */
struct list_head list;  /* hvt_list */
int (*on_msg)(void *, int); /* callback on new user message */
-- 
1.7.1

[PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't

2017-02-27 Thread kys 
From: Dexuan Cui 

If the daemon is NOT running at all, when we disable the util device from
Hyper-V Manager (or sometimes the host can rescind a util device and then
re-offer it), we'll hang in util_remove -> hv_kvp_deinit ->
wait_for_completion(_event), because this code path doesn't run:
hvt_op_release -> ... -> kvp_on_reset -> complete(_event).

Due to this, we even can't reboot the VM properly.

The patch tracks if the dev file is opened or not, and we only need to
wait if it's opened.

Fixes: 5a66fecbf6aa ("Drivers: hv: util: kvp: Fix a rescind processing issue")

Signed-off-by: Dexuan Cui 
Cc: Vitaly Kuznetsov 
Cc: "K. Y. Srinivasan" 
Cc: Haiyang Zhang 
Cc: Stephen Hemminger 
Signed-off-by: K. Y. Srinivasan 
---
 drivers/hv/hv_fcopy.c   |5 -
 drivers/hv/hv_kvp.c |6 +-
 drivers/hv/hv_snapshot.c|5 -
 drivers/hv/hv_utils_transport.c |2 ++
 drivers/hv/hv_utils_transport.h |1 +
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index 9aee601..545cf43 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -358,8 +358,11 @@ int hv_fcopy_init(struct hv_util_service *srv)
 
 void hv_fcopy_deinit(void)
 {
+   bool wait = hvt->dev_opened;
+
fcopy_transaction.state = HVUTIL_DEVICE_DYING;
cancel_delayed_work_sync(_timeout_work);
hvutil_transport_destroy(hvt);
-   wait_for_completion(_event);
+   if (wait)
+   wait_for_completion(_event);
 }
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index de26371..15c7873 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -742,10 +742,14 @@ static void kvp_on_reset(void)
 
 void hv_kvp_deinit(void)
 {
+   bool wait = hvt->dev_opened;
+
kvp_transaction.state = HVUTIL_DEVICE_DYING;
cancel_delayed_work_sync(_host_handshake_work);
cancel_delayed_work_sync(_timeout_work);
cancel_work_sync(_sendkey_work);
hvutil_transport_destroy(hvt);
-   wait_for_completion(_event);
+
+   if (wait)
+   wait_for_completion(_event);
 }
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index bcc03f0..3847f19 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -396,9 +396,12 @@ static void vss_on_reset(void)
 
 void hv_vss_deinit(void)
 {
+   bool wait = hvt->dev_opened;
+
vss_transaction.state = HVUTIL_DEVICE_DYING;
cancel_delayed_work_sync(_timeout_work);
cancel_work_sync(_handle_request_work);
hvutil_transport_destroy(hvt);
-   wait_for_completion(_event);
+   if (wait)
+   wait_for_completion(_event);
 }
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index c235a95..05e0648 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -153,6 +153,7 @@ static int hvt_op_open(struct inode *inode, struct file 
*file)
 
if (issue_reset)
hvt_reset(hvt);
+   hvt->dev_opened = (hvt->mode == HVUTIL_TRANSPORT_CHARDEV) && !ret;
 
mutex_unlock(>lock);
 
@@ -182,6 +183,7 @@ static int hvt_op_release(struct inode *inode, struct file 
*file)
 * connects back.
 */
hvt_reset(hvt);
+   hvt->dev_opened = false;
mutex_unlock(>lock);
 
if (mode_old == HVUTIL_TRANSPORT_DESTROY)
diff --git a/drivers/hv/hv_utils_transport.h b/drivers/hv/hv_utils_transport.h
index d98f522..9871283 100644
--- a/drivers/hv/hv_utils_transport.h
+++ b/drivers/hv/hv_utils_transport.h
@@ -32,6 +32,7 @@ struct hvutil_transport {
int mode;   /* hvutil_transport_mode */
struct file_operations fops;/* file operations */
struct miscdevice mdev; /* misc device */
+   bool   dev_opened;  /* Is the device opened? */
struct cb_id cn_id; /* CN_*_IDX/CN_*_VAL */
struct list_head list;  /* hvt_list */
int (*on_msg)(void *, int); /* callback on new user message */
-- 
1.7.1