KY Srinivasan <kys@xxxxxxxxxxxxx> writes: >> -----Original Message----- >> From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx] >> Sent: Monday, July 13, 2015 5:19 AM >> To: devel@xxxxxxxxxxxxxxxxxxxxxx >> Cc: KY Srinivasan; Haiyang Zhang; Dexuan Cui; linux-kernel@xxxxxxxxxxxxxxx >> 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(). 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@xxxxxxxxxx> > > Thanks Vitaly; strangely enough, I too am looking at this exact problem. I was planning to > address this problem a little differently: I was planning to hold the context that performed > the initial (primary) channel open until all of the "open activity" was completed and this would include > opening of the sub-channels for multi-channel devices. (not sure it can actually happen with current implementation of Hyper-V host but) in case a subchannel is rescinded and reopened again later on such 'full open lock' won't probably help but in other cases both approaches should give us equal results. This is not a hotpath, any syncronization should do the job. > > Regards, > > K. Y > >> --- >> 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 -- Vitaly _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
