From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751734AbbGMMS7 (ORCPT <rfc822;w@1wt.eu>);
	Mon, 13 Jul 2015 08:18:59 -0400
Received: from mx1.redhat.com ([209.132.183.28]:43539 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751123AbbGMMS6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 13 Jul 2015 08:18:58 -0400
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: devel@linuxdriverproject.org
Cc: "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Dexuan Cui <decui@microsoft.com>, linux-kernel@vger.kernel.org
Subject: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown
Date: Mon, 13 Jul 2015 14:18:54 +0200
Message-Id: <1436789934-11566-1-git-send-email-vkuznets@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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