Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1588616 > unrolled thread
| Started by | Dexuan Cui <decui@microsoft.com> |
|---|---|
| First post | 2017-02-27 12:30 +0100 |
| Last post | 2017-02-28 14:00 +0100 |
| Articles | 8 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Dexuan Cui <decui@microsoft.com> - 2017-02-27 12:30 +0100
RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Stephen Hemminger <sthemmin@microsoft.com> - 2017-02-27 18:20 +0100
RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Dexuan Cui <decui@microsoft.com> - 2017-02-28 03:40 +0100
RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Dexuan Cui <decui@microsoft.com> - 2017-02-28 13:50 +0100
RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Dexuan Cui <decui@microsoft.com> - 2017-02-28 14:20 +0100
Re: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-02-28 14:20 +0100
Re: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-02-28 15:30 +0100
Re: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-02-28 14:00 +0100
| From | Dexuan Cui <decui@microsoft.com> |
|---|---|
| Date | 2017-02-27 12:30 +0100 |
| Subject | [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfrF7-4Ql-27@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Stephen Hemminger <sthemmin@microsoft.com> |
|---|---|
| Date | 2017-02-27 18:20 +0100 |
| Subject | RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfx7P-gn-7@gated-at.bofh.it> |
| In reply to | #1588616 |
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
[toc] | [prev] | [next] | [standalone]
| From | Dexuan Cui <decui@microsoft.com> |
|---|---|
| Date | 2017-02-28 03:40 +0100 |
| Subject | RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfFRL-6hE-19@gated-at.bofh.it> |
| In reply to | #1588823 |
> 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
[toc] | [prev] | [next] | [standalone]
| From | Dexuan Cui <decui@microsoft.com> |
|---|---|
| Date | 2017-02-28 13:50 +0100 |
| Subject | RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfPo6-4t2-3@gated-at.bofh.it> |
| In reply to | #1588616 |
> 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
[toc] | [prev] | [next] | [standalone]
| From | Dexuan Cui <decui@microsoft.com> |
|---|---|
| Date | 2017-02-28 14:20 +0100 |
| Subject | RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfPR7-4Up-1@gated-at.bofh.it> |
| In reply to | #1589413 |
> 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
[toc] | [prev] | [next] | [standalone]
| From | Vitaly Kuznetsov <vkuznets@redhat.com> |
|---|---|
| Date | 2017-02-28 14:20 +0100 |
| Subject | Re: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfPR7-4Up-5@gated-at.bofh.it> |
| In reply to | #1589427 |
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
[toc] | [prev] | [next] | [standalone]
| From | Vitaly Kuznetsov <vkuznets@redhat.com> |
|---|---|
| Date | 2017-02-28 15:30 +0100 |
| Subject | Re: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfQWR-5Cn-7@gated-at.bofh.it> |
| In reply to | #1589428 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Vitaly Kuznetsov <vkuznets@redhat.com> |
|---|---|
| Date | 2017-02-28 14:00 +0100 |
| Subject | Re: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't |
| Message-ID | <tfPo6-4t2-5@gated-at.bofh.it> |
| In reply to | #1588616 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web