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

[KY Srinivasan]

> -----Original Message-----
> From: kys@xxxxxxxxxxxxxxxxxxxxxx [mailto:kys@xxxxxxxxxxxxxxxxxxxxxx]
> Sent: Monday, February 27, 2017 3:18 PM
> To: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx;
> leann.ogasawara@xxxxxxxxxxxxx
> Cc: Dexuan Cui <decui@xxxxxxxxxxxxx>; KY Srinivasan
> <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Stephen
> Hemminger <sthemmin@xxxxxxxxxxxxx>
> Subject: [PATCH 1/1] Drivers: hv: util: on deinit, don't wait the release event,
> if we shouldn't
> 
> This&nbsp;sender&nbsp;failed&nbsp;our&nbsp;fraud&nbsp;detection&nbs
> p;checks&nbsp;and&nbsp;may&nbsp;not&nbsp;be&nbsp;who&nbsp;they&
> nbsp;appear&nbsp;to&nbsp;be.&nbsp;Learn&nbsp;about&nbsp;<a
> href="http://aka.ms/LearnAboutSpoofing";>spoofing</a>
> 
> From: Dexuan Cui <decui@xxxxxxxxxxxxx>
> 
> 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(&release_event), because this code path doesn't run:
> hvt_op_release -> ... -> kvp_on_reset -> complete(&release_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 <decui@xxxxxxxxxxxxx>
> Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> Cc: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx>
> Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Cc: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>
> Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> ---
>  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(&fcopy_timeout_work);
>         hvutil_transport_destroy(hvt);
> -       wait_for_completion(&release_event);
> +       if (wait)
> +               wait_for_completion(&release_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(&kvp_host_handshake_work);
>         cancel_delayed_work_sync(&kvp_timeout_work);
>         cancel_work_sync(&kvp_sendkey_work);
>         hvutil_transport_destroy(hvt);
> -       wait_for_completion(&release_event);
> +
> +       if (wait)
> +               wait_for_completion(&release_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(&vss_timeout_work);
>         cancel_work_sync(&vss_handle_request_work);
>         hvutil_transport_destroy(hvt);
> -       wait_for_completion(&release_event);
> +       if (wait)
> +               wait_for_completion(&release_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(&hvt->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(&hvt->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

Greg,

Please drop this patch.

K. Y