> -----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. 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
