Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1182761 > unrolled thread
| Started by | Vitaly Kuznetsov <vkuznets@redhat.com> |
|---|---|
| First post | 2015-07-13 14:20 +0200 |
| Last post | 2015-07-16 02:40 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-07-13 14:20 +0200
Re: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-07-14 18:10 +0200
RE: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown KY Srinivasan <kys@microsoft.com> - 2015-07-16 02:40 +0200
| From | Vitaly Kuznetsov <vkuznets@redhat.com> |
|---|---|
| Date | 2015-07-13 14:20 +0200 |
| Subject | [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown |
| Message-ID | <pLKSe-4Ui-35@gated-at.bofh.it> |
When a new subchannel offer from host comes during device shutdown (e.g.
when a netvsc/storvsc module is unloadedshortly after it was loaded) a
crash can happen as vmbus_process_offer() is not anyhow serialized with
vmbus_remove(). As an example we can try calling subchannel create
callback when the module is already unloaded.
The following crash was observed while keeping loading/unloading hv_netvsc
module on 64-CPU guest:
hv_netvsc vmbus_14: net device safe to remove
BUG: unable to handle kernel paging request at 000000000000a118
IP: [<ffffffffa01c1b21>] netvsc_sc_open+0x31/0xb0 [hv_netvsc]
PGD 1f3946a067 PUD 1f38a5f067 PMD 0
Oops: 0000 [#1] SMP
...
Call Trace:
[<ffffffffa0055cd7>] vmbus_onoffer+0x477/0x530 [hv_vmbus]
[<ffffffff81092e1f>] ? move_linked_works+0x5f/0x80
[<ffffffffa0055fd3>] vmbus_onmessage+0x33/0xa0 [hv_vmbus]
[<ffffffffa0052f81>] vmbus_onmessage_work+0x21/0x30 [hv_vmbus]
[<ffffffff81095cde>] process_one_work+0x18e/0x4e0
...
The issue cannot be solved by just resetting sc_creation_callback on
driver removal as while we search for the parent channel with channel_lock
held we release it after the channel was found and it can disapper beneath
our feet while we're still in vmbus_process_offer();
Introduce new sc_create_lock mutex and take it in vmbus_remove() to ensure
no new subchannels are created after we started the removal procedure.
Check its state with mutex_trylock in vmbus_process_offer().
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/channel.c | 3 +++
drivers/hv/channel_mgmt.c | 20 ++++++++++++++++++--
drivers/hv/vmbus_drv.c | 6 ++++++
include/linux/hyperv.h | 4 ++++
4 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 603ce97..faa91fe 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -555,6 +555,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
if (channel->rescind)
hv_process_channel_removal(channel,
channel->offermsg.child_relid);
+ else if (mutex_is_locked(&channel->sc_create_lock))
+ mutex_unlock(&channel->sc_create_lock);
+
return ret;
}
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 4506a66..96f8b96 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -150,6 +150,8 @@ static struct vmbus_channel *alloc_channel(void)
INIT_LIST_HEAD(&channel->sc_list);
INIT_LIST_HEAD(&channel->percpu_list);
+ mutex_init(&channel->sc_create_lock);
+
return channel;
}
@@ -158,6 +160,7 @@ static struct vmbus_channel *alloc_channel(void)
*/
static void free_channel(struct vmbus_channel *channel)
{
+ mutex_destroy(&channel->sc_create_lock);
kfree(channel);
}
@@ -247,8 +250,18 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
newchannel->offermsg.offer.if_type) &&
!uuid_le_cmp(channel->offermsg.offer.if_instance,
newchannel->offermsg.offer.if_instance)) {
- fnew = false;
- break;
+ if (mutex_trylock(&channel->sc_create_lock)) {
+ fnew = false;
+ break;
+ }
+ /*
+ * If we fail to acquire mutex here it means we're
+ * closing parent channel at this moment. We can't
+ * create new subchannel in this case.
+ */
+ spin_unlock_irqrestore(&vmbus_connection.channel_lock,
+ flags);
+ goto err_free_chan;
}
}
@@ -297,6 +310,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
if (!fnew) {
if (channel->sc_creation_callback != NULL)
channel->sc_creation_callback(newchannel);
+ mutex_unlock(&channel->sc_create_lock);
return;
}
@@ -340,6 +354,8 @@ err_deq_chan:
}
err_free_chan:
+ if (!fnew)
+ mutex_unlock(&channel->sc_create_lock);
free_channel(newchannel);
}
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index cf20400..7ad7fcc 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -539,6 +539,12 @@ static int vmbus_remove(struct device *child_device)
struct hv_device *dev = device_to_hv_device(child_device);
u32 relid = dev->channel->offermsg.child_relid;
+ /*
+ * Device is being removed, prevent creating new subchannels. Mutex
+ * will be released in vmbus_close_internal().
+ */
+ mutex_lock(&dev->channel->sc_create_lock);
+
if (child_device->driver) {
drv = drv_to_hv_drv(child_device->driver);
if (drv->remove)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 30d3a1f..f1af05c 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -748,6 +748,10 @@ struct vmbus_channel {
*/
struct vmbus_channel *primary_channel;
/*
+ * Protects sub-channel create path.
+ */
+ struct mutex sc_create_lock;
+ /*
* Support per-channel state for use by vmbus drivers.
*/
void *per_channel_state;
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Vitaly Kuznetsov <vkuznets@redhat.com> |
|---|---|
| Date | 2015-07-14 18:10 +0200 |
| Message-ID | <pMaWm-5cM-25@gated-at.bofh.it> |
| In reply to | #1182761 |
Dexuan Cui <decui@microsoft.com> writes:
>> -----Original Message-----
>> From: Vitaly Kuznetsov
>> Sent: Monday, July 13, 2015 20:19
>> Subject: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device
>> shutdown
>>
>> When a new subchannel offer from host comes during device shutdown (e.g.
>> when a netvsc/storvsc module is unloadedshortly after it was loaded) a
>> crash can happen as vmbus_process_offer() is not anyhow serialized with
>> vmbus_remove().
>
> How about vmbus_onoffer_rescind()?
> It's not serialized with vmbus_remove() either, so I think there is an issue too?
>
> I remember when 'rmmod hv_netvsc', we get a rescind-offer message for
> each subchannel.
>
True, I think we have a race with rescind messages as well, we just
never saw crashes for some reason. I'll think how we can make the fix
more general.
>> As an example we can try calling subchannel create
>> callback when the module is already unloaded.
>> The following crash was observed while keeping loading/unloading hv_netvsc
>> module on 64-CPU guest:
>>
>> hv_netvsc vmbus_14: net device safe to remove
>> BUG: unable to handle kernel paging request at 000000000000a118
>> IP: [<ffffffffa01c1b21>] netvsc_sc_open+0x31/0xb0 [hv_netvsc]
>> PGD 1f3946a067 PUD 1f38a5f067 PMD 0
>> Oops: 0000 [#1] SMP
>> ...
>> Call Trace:
>> [<ffffffffa0055cd7>] vmbus_onoffer+0x477/0x530 [hv_vmbus]
>> [<ffffffff81092e1f>] ? move_linked_works+0x5f/0x80
>> [<ffffffffa0055fd3>] vmbus_onmessage+0x33/0xa0 [hv_vmbus]
>> [<ffffffffa0052f81>] vmbus_onmessage_work+0x21/0x30 [hv_vmbus]
>> [<ffffffff81095cde>] process_one_work+0x18e/0x4e0
>> ...
>>
>> The issue cannot be solved by just resetting sc_creation_callback on
>> driver removal as while we search for the parent channel with channel_lock
>> held we release it after the channel was found and it can disapper beneath
>> our feet while we're still in vmbus_process_offer();
>>
>> Introduce new sc_create_lock mutex and take it in vmbus_remove() to ensure
>> no new subchannels are created after we started the removal procedure.
>> Check its state with mutex_trylock in vmbus_process_offer().
>
> In my 8-CPU VM, I can very easily reproduce the panic by
> 1. running
> while ((1)); do modprobe -r hv_netvsc; modprobe hv_netvsc; sleep 10; done.
>
> and
> 2. in vmbus_onoffer_rescind(), we sleep 3s after a subchannel is added into
> the primary channel's sc_list (and before the sc_creation_callback is invoked):
> (I added line 275)
>
> 262 if (!fnew) {
> 263 /*
> 264 * Check to see if this is a sub-channel.
> 265 */
> 266 if (newchannel->offermsg.offer.sub_channel_index != 0) {
> 267 /*
> 268 * Process the sub-channel.
> 269 */
> 270 newchannel->primary_channel = channel;
> 271 spin_lock_irqsave(&channel->lock, flags);
> 272 list_add_tail(&newchannel->sc_list, &channel->sc_list);
> 273 channel->num_sc++;
> 274 spin_unlock_irqrestore(&channel->lock, flags);
> 275 ssleep(3);
> 276 } else
> 277 goto err_free_chan;
> 278 }
It is possible to see crashes even without such intrumentation, move
CPUs and slower host will do the job.
Thanks,
--
Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | KY Srinivasan <kys@microsoft.com> |
|---|---|
| Date | 2015-07-16 02:40 +0200 |
| Subject | RE: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown |
| Message-ID | <pMFnt-7jk-25@gated-at.bofh.it> |
| In reply to | #1183824 |
> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > Sent: Tuesday, July 14, 2015 9:03 AM > To: Dexuan Cui > Cc: devel@linuxdriverproject.org; KY Srinivasan; Haiyang Zhang; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation > on device shutdown > > Dexuan Cui <decui@microsoft.com> writes: > > >> -----Original Message----- > >> From: Vitaly Kuznetsov > >> Sent: Monday, July 13, 2015 20:19 > >> Subject: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on > device > >> shutdown > >> > >> When a new subchannel offer from host comes during device shutdown > (e.g. > >> when a netvsc/storvsc module is unloadedshortly after it was loaded) a > >> crash can happen as vmbus_process_offer() is not anyhow serialized with > >> vmbus_remove(). > > > > How about vmbus_onoffer_rescind()? > > It's not serialized with vmbus_remove() either, so I think there is an issue > too? > > > > I remember when 'rmmod hv_netvsc', we get a rescind-offer message for > > each subchannel. > > > > True, I think we have a race with rescind messages as well, we just > never saw crashes for some reason. I'll think how we can make the fix > more general. In an earlier email I had outlined how I was planning to address original issue. I have a patch based on the logic I had described - essentially put the burden on the probe call to ensure that the open (including sub-channels) is complete before we return from probe. I will post this shortly. Regards, K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web