[PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[Vitaly Kuznetsov]

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

RE: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, July 13, 2015 5:19 AM
> To: devel@linuxdriverproject.org
> Cc: KY Srinivasan; Haiyang Zhang; Dexuan Cui; linux-kernel@vger.kernel.org
> 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@redhat.com>

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

Re: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[Vitaly Kuznetsov]

KY Srinivasan <kys@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Monday, July 13, 2015 5:19 AM
>> To: devel@linuxdriverproject.org
>> Cc: KY Srinivasan; Haiyang Zhang; Dexuan Cui; linux-kernel@vger.kernel.org
>> 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@redhat.com>
>
> 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

RE: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[Dexuan Cui]

> -----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.

> 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         }

Re: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[Vitaly Kuznetsov]

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

RE: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[KY Srinivasan]

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

RE: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown

[KY Srinivasan]

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