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

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

[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(&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@microsoft.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
---
 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 @@ hv_kvp_init(struct hv_util_service *srv)
 
 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 @@ hv_vss_init(struct hv_util_service *srv)
 
 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 */
-- 
2.7.4

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

[Stephen Hemminger]

The patch looks good. I don't  understand the exact semantics here and therefore have a couple of naïve questions

Is it possible device could be opened multiple times? Is reference counting done above?
If device was never opened do you even have to do all the cleanup steps at all?

-----Original Message-----
From: Dexuan Cui 
Sent: Monday, February 27, 2017 3:26 AM
To: gregkh@linuxfoundation.org; driverdev-devel@linuxdriverproject.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>; linux-kernel@vger.kernel.org; Alex Ng (LIS) <alexng@microsoft.com>
Subject: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't


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@microsoft.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
---
 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 @@ hv_kvp_init(struct hv_util_service *srv)
 
 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 @@ hv_vss_init(struct hv_util_service *srv)
 
 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 */
-- 
2.7.4

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

[Dexuan Cui]

> From: Stephen Hemminger
> Sent: Tuesday, February 28, 2017 01:02
> 
> The patch looks good. I don't  understand the exact semantics here and
> therefore have a couple of naïve questions
> 
> Is it possible device could be opened multiple times? Is reference counting
> done above?

The file can only be opened once.
When we open it for the first time, in hvt_op_open() we run
hvt->mode = HVUTIL_TRANSPORT_CHARDEV, and when we try to open it later,
the open() gets -EBUSY.
When the first fd is closed, in hvt_op_release() we run 
hvt->mode = HVUTIL_TRANSPORT_INIT, so we can re-open it successfully later.

> If device was never opened do you even have to do all the cleanup steps at
> all?

Yes, e.g. we do need the 3 cancel_*work_sync calls in hv_kvp_deinit():
the driver schedules the 3 works even if there is no daemon at all, e.g.
hv_kvp_onchannelcallback ->
    schedule_delayed_work(&kvp_host_handshake_work, ...).

Thanks,
-- Dexuan

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

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

> 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@microsoft.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  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);

This is racy I think. We need to prevent openning the device first and
then query its state:

	bool wait;

 	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
        /* make sure state is set */
        mb();
        wait = hvt->dev_opened;
  	cancel_delayed_work_sync(&fcopy_timeout_work);
  	hvutil_transport_destroy(hvt);
        if (wait)
		wait_for_completion(&release_event);

otherwise someone could open the device before we manage to update its
state.

>  }
> 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 @@ hv_kvp_init(struct hv_util_service *srv)
>
>  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 @@ hv_vss_init(struct hv_util_service *srv)
>
>  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);
>

Not sure but it seems this may also be racy (what if we query the state
just before we reset it?).

>  	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 */

I think we can get away without introducing this new flag, e.g. if we
replace release_event with an atomic which will hold the state
(open/closed). This will also elimenate possible races above. I can try
prototyping a patch if you want me to.

Thanks,

-- 
  Vitaly

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

[Dexuan Cui]

> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >  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);
> 
> This is racy I think. We need to prevent openning the device first and
> then query its state:
> 
> 	bool wait;
> 
>  	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
>         /* make sure state is set */
>         mb();
>         wait = hvt->dev_opened;
>   	cancel_delayed_work_sync(&fcopy_timeout_work);
>   	hvutil_transport_destroy(hvt);
>         if (wait)
> 		wait_for_completion(&release_event);
> 
> otherwise someone could open the device before we manage to update its
> state.

I agree.

> > @@ -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);
> >
> 
> Not sure but it seems this may also be racy (what if we query the state
> just before we reset it?).

Yeah, I agree.

> >  	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 */
> 
> I think we can get away without introducing this new flag, e.g. if we
> replace release_event with an atomic which will hold the state
> (open/closed). This will also elimenate possible races above. I can try
> prototyping a patch if you want me to.
> --
>   Vitaly

Thanks for offering the help! Please do. :-)

Thanks,
-- Dexuan

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

[Dexuan Cui]

> From: devel [...] On Behalf Of Dexuan Cui
> > > --- 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 */
> >
> > I think we can get away without introducing this new flag, e.g. if we
> > replace release_event with an atomic which will hold the state
> > (open/closed). This will also elimenate possible races above. I can try
> > prototyping a patch if you want me to.
> > --
> >   Vitaly
> 
> Thanks for offering the help! Please do. :-)

BTW, IMO I found another potential issue:
In hvt_op_open -> hvt_reset -> kvp_on_reset(), I think we should call
init_completion() instead of complete()?

Thanks,
-- Dexuan

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

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

>> From: devel [...] On Behalf Of Dexuan Cui
>> > > --- 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 */
>> >
>> > I think we can get away without introducing this new flag, e.g. if we
>> > replace release_event with an atomic which will hold the state
>> > (open/closed). This will also elimenate possible races above. I can try
>> > prototyping a patch if you want me to.
>> > --
>> >   Vitaly
>> 
>> Thanks for offering the help! Please do. :-)
>
> BTW, IMO I found another potential issue:
> In hvt_op_open -> hvt_reset -> kvp_on_reset(), I think we should call
> init_completion() instead of complete()?
>

To me it looks like we can do better with something different from
struct completion, I'll take a look later today.

-- 
  Vitaly

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

[Vitaly Kuznetsov]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Dexuan Cui <decui@microsoft.com> writes:
>
>>> From: devel [...] On Behalf Of Dexuan Cui
>>> > > --- 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 */
>>> >
>>> > I think we can get away without introducing this new flag, e.g. if we
>>> > replace release_event with an atomic which will hold the state
>>> > (open/closed). This will also elimenate possible races above. I can try
>>> > prototyping a patch if you want me to.
>>> > --
>>> >   Vitaly
>>> 
>>> Thanks for offering the help! Please do. :-)
>>
>> BTW, IMO I found another potential issue:
>> In hvt_op_open -> hvt_reset -> kvp_on_reset(), I think we should call
>> init_completion() instead of complete()?
>>
>
> To me it looks like we can do better with something different from
> struct completion, I'll take a look later today.

Dexuan,

please take a look at the attached patch. After looking at the code
again it occured to me that it's going to be easier to move release wait
to the transport itself. Lightly tested.

Thanks!

-- 
  Vitaly


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Drivers-hv-util-move-waiting-for-release-to-hv_utils.patch --]
[-- Type: text/x-patch, Size: 6425 bytes --]

>From e1144c6127d4f9acffeb64f40449de365caec51e Mon Sep 17 00:00:00 2001
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Tue, 28 Feb 2017 15:12:17 +0100
Subject: [PATCH] Drivers: hv: util: move waiting for release to
 hv_utils_transport itself

Waiting for release_event in all three drivers introduced issues on release
as on_reset() hook is not always called. E.g. if the device was never
opened we will never get the completion.

Move the waiting code to hvutil_transport_destroy() and make sure it is
only called when the device is open. hvt->lock serialization should
guarantee the absence of races.

Fixes: 5a66fecbf6aa ("Drivers: hv: util: kvp: Fix a rescind processing issue")
Fixes: 20951c7535b5 ("Drivers: hv: util: Fcopy: Fix a rescind processing issue")
Fixes: d77044d142e9 ("Drivers: hv: util: Backup: Fix a rescind processing issue")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/hv_fcopy.c           |  4 ----
 drivers/hv/hv_kvp.c             |  4 ----
 drivers/hv/hv_snapshot.c        |  4 ----
 drivers/hv/hv_utils_transport.c | 12 ++++++++----
 drivers/hv/hv_utils_transport.h |  1 +
 5 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index 9aee601..a5596a6 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -71,7 +71,6 @@ static DECLARE_WORK(fcopy_send_work, fcopy_send_data);
 static const char fcopy_devname[] = "vmbus/hv_fcopy";
 static u8 *recv_buffer;
 static struct hvutil_transport *hvt;
-static struct completion release_event;
 /*
  * This state maintains the version number registered by the daemon.
  */
@@ -331,7 +330,6 @@ static void fcopy_on_reset(void)
 
 	if (cancel_delayed_work_sync(&fcopy_timeout_work))
 		fcopy_respond_to_host(HV_E_FAIL);
-	complete(&release_event);
 }
 
 int hv_fcopy_init(struct hv_util_service *srv)
@@ -339,7 +337,6 @@ int hv_fcopy_init(struct hv_util_service *srv)
 	recv_buffer = srv->recv_buffer;
 	fcopy_transaction.recv_channel = srv->channel;
 
-	init_completion(&release_event);
 	/*
 	 * When this driver loads, the user level daemon that
 	 * processes the host requests may not yet be running.
@@ -361,5 +358,4 @@ void hv_fcopy_deinit(void)
 	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
 	cancel_delayed_work_sync(&fcopy_timeout_work);
 	hvutil_transport_destroy(hvt);
-	wait_for_completion(&release_event);
 }
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index de26371..a1adfe2 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -101,7 +101,6 @@ static DECLARE_WORK(kvp_sendkey_work, kvp_send_key);
 static const char kvp_devname[] = "vmbus/hv_kvp";
 static u8 *recv_buffer;
 static struct hvutil_transport *hvt;
-static struct completion release_event;
 /*
  * Register the kernel component with the user-level daemon.
  * As part of this registration, pass the LIC version number.
@@ -714,7 +713,6 @@ static void kvp_on_reset(void)
 	if (cancel_delayed_work_sync(&kvp_timeout_work))
 		kvp_respond_to_host(NULL, HV_E_FAIL);
 	kvp_transaction.state = HVUTIL_DEVICE_INIT;
-	complete(&release_event);
 }
 
 int
@@ -723,7 +721,6 @@ hv_kvp_init(struct hv_util_service *srv)
 	recv_buffer = srv->recv_buffer;
 	kvp_transaction.recv_channel = srv->channel;
 
-	init_completion(&release_event);
 	/*
 	 * When this driver loads, the user level daemon that
 	 * processes the host requests may not yet be running.
@@ -747,5 +744,4 @@ void hv_kvp_deinit(void)
 	cancel_delayed_work_sync(&kvp_timeout_work);
 	cancel_work_sync(&kvp_sendkey_work);
 	hvutil_transport_destroy(hvt);
-	wait_for_completion(&release_event);
 }
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index bcc03f0..e659d1b 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -79,7 +79,6 @@ static int dm_reg_value;
 static const char vss_devname[] = "vmbus/hv_vss";
 static __u8 *recv_buffer;
 static struct hvutil_transport *hvt;
-static struct completion release_event;
 
 static void vss_timeout_func(struct work_struct *dummy);
 static void vss_handle_request(struct work_struct *dummy);
@@ -361,13 +360,11 @@ static void vss_on_reset(void)
 	if (cancel_delayed_work_sync(&vss_timeout_work))
 		vss_respond_to_host(HV_E_FAIL);
 	vss_transaction.state = HVUTIL_DEVICE_INIT;
-	complete(&release_event);
 }
 
 int
 hv_vss_init(struct hv_util_service *srv)
 {
-	init_completion(&release_event);
 	if (vmbus_proto_version < VERSION_WIN8_1) {
 		pr_warn("Integration service 'Backup (volume snapshot)'"
 			" not supported on this host version.\n");
@@ -400,5 +397,4 @@ void hv_vss_deinit(void)
 	cancel_delayed_work_sync(&vss_timeout_work);
 	cancel_work_sync(&vss_handle_request_work);
 	hvutil_transport_destroy(hvt);
-	wait_for_completion(&release_event);
 }
diff --git a/drivers/hv/hv_utils_transport.c b/drivers/hv/hv_utils_transport.c
index c235a95..4402a71 100644
--- a/drivers/hv/hv_utils_transport.c
+++ b/drivers/hv/hv_utils_transport.c
@@ -182,10 +182,11 @@ static int hvt_op_release(struct inode *inode, struct file *file)
 	 * connects back.
 	 */
 	hvt_reset(hvt);
-	mutex_unlock(&hvt->lock);
 
 	if (mode_old == HVUTIL_TRANSPORT_DESTROY)
-		hvt_transport_free(hvt);
+		complete(&hvt->release);
+
+	mutex_unlock(&hvt->lock);
 
 	return 0;
 }
@@ -304,6 +305,7 @@ struct hvutil_transport *hvutil_transport_init(const char *name,
 
 	init_waitqueue_head(&hvt->outmsg_q);
 	mutex_init(&hvt->lock);
+	init_completion(&hvt->release);
 
 	spin_lock(&hvt_list_lock);
 	list_add(&hvt->list, &hvt_list);
@@ -351,6 +353,8 @@ void hvutil_transport_destroy(struct hvutil_transport *hvt)
 	if (hvt->cn_id.idx > 0 && hvt->cn_id.val > 0)
 		cn_del_callback(&hvt->cn_id);
 
-	if (mode_old != HVUTIL_TRANSPORT_CHARDEV)
-		hvt_transport_free(hvt);
+	if (mode_old == HVUTIL_TRANSPORT_CHARDEV)
+		wait_for_completion(&hvt->release);
+
+	hvt_transport_free(hvt);
 }
diff --git a/drivers/hv/hv_utils_transport.h b/drivers/hv/hv_utils_transport.h
index d98f522..79afb62 100644
--- a/drivers/hv/hv_utils_transport.h
+++ b/drivers/hv/hv_utils_transport.h
@@ -41,6 +41,7 @@ struct hvutil_transport {
 	int outmsg_len;                     /* its length */
 	wait_queue_head_t outmsg_q;         /* poll/read wait queue */
 	struct mutex lock;                  /* protects struct members */
+	struct completion release;          /* synchronize with fd release */
 };
 
 struct hvutil_transport *hvutil_transport_init(const char *name,
-- 
2.9.3

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

[Dexuan Cui]

> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
(c)
> >>> > >     int (*on_msg)(void *, int);         /* callback on new user message */
> >>> >
> >>> > I think we can get away without introducing this new flag, e.g. if we
> >>> > replace release_event with an atomic which will hold the state
> >>> > (open/closed). This will also elimenate possible races above. I can try
> >>> > prototyping a patch if you want me to.
> >>> > --
> >>> >   Vitaly
> >>>
> >>> Thanks for offering the help! Please do. :-)
> >>
> >> BTW, IMO I found another potential issue:
> >> In hvt_op_open -> hvt_reset -> kvp_on_reset(), I think we should call
> >> init_completion() instead of complete()?
> >>
> >
> > To me it looks like we can do better with something different from
> > struct completion, I'll take a look later today.
> 
> Dexuan,
> 
> please take a look at the attached patch. After looking at the code
> again it occured to me that it's going to be easier to move release wait
> to the transport itself. Lightly tested.
> 
>   Vitaly

You made a neat patch, which can fix the bug.

Thank you a lot!

Thanks,
-- Dexuan