thr3ads.net - Virtualization - [PATCH 1/2] staging: hv: Fix race condition in hv

If this information is useful, please help other people find it:
Share via:

Haiyang Zhang

2010-May-19 15:54 UTC

[PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.

From: Haiyang Zhang <haiyangz at microsoft.com>

Subject: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module
initialization.
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an atomic counter to ensure the hv_utils module initialization
happens after all vmbus IC channels are initialized.

Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
Signed-off-by: Hank Janssen <hjanssen at microsoft.com>

---
 drivers/staging/hv/channel_mgmt.c |   26 +++++++++++++++-----------
 drivers/staging/hv/hv_utils.c     |   11 ++++++++---
 drivers/staging/hv/utils.h        |    5 +++++
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c
b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..b5b6a70 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -33,7 +33,6 @@ struct vmbus_channel_message_table_entry {
 	void (*messageHandler)(struct vmbus_channel_message_header *msg);
 };
 
-#define MAX_MSG_TYPES                    3
 #define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
 
 static const struct hv_guid
@@ -233,6 +232,11 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES] =
{
 };
 EXPORT_SYMBOL(hv_cb_utils);
 
+/* Counter of IC channels initialized */
+atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
+EXPORT_SYMBOL(hv_utils_initcnt);
+
+
 /*
  * AllocVmbusChannel - Allocate and initialize a vmbus channel object
  */
@@ -373,22 +377,22 @@ static void VmbusChannelProcessOffer(void *context)
 		 * can cleanup properly
 		 */
 		newChannel->State = CHANNEL_OPEN_STATE;
-		cnt = 0;
 
-		while (cnt != MAX_MSG_TYPES) {
+		/* Open IC channels */
+		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
 			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
 				   &hv_cb_utils[cnt].data,
-				   sizeof(struct hv_guid)) == 0) {
+				   sizeof(struct hv_guid)) == 0 &&
+			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+					     2 * PAGE_SIZE, NULL, 0,
+					     hv_cb_utils[cnt].callback,
+					     newChannel) == 0) {
+				hv_cb_utils[cnt].channel = newChannel;
+				mb();
 				DPRINT_INFO(VMBUS, "%s",
 					    hv_cb_utils[cnt].log_msg);
-
-				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
-						    2 * PAGE_SIZE, NULL, 0,
-						    hv_cb_utils[cnt].callback,
-						    newChannel) == 0)
-					hv_cb_utils[cnt].channel = newChannel;
+				atomic_inc(&hv_utils_initcnt);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8a49aaf..f9826c7 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -253,7 +253,11 @@ static void heartbeat_onchannelcallback(void *context)
 
 static int __init init_hyperv_utils(void)
 {
-	printk(KERN_INFO "Registering HyperV Utility Driver\n");
+	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
+
+	/* Wait until all IC channels are initialized */
+	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
+		msleep(100);
 
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback  	
&shutdown_onchannelcallback;
@@ -267,13 +271,12 @@ static int __init init_hyperv_utils(void)
 		&heartbeat_onchannelcallback;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
 
+	printk(KERN_INFO "Registered HyperV Utility Driver.\n");
 	return 0;
 }
 
 static void exit_hyperv_utils(void)
 {
-	printk(KERN_INFO "De-Registered HyperV Utility Driver\n");
-
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback  	
&chn_cb_negotiate;
 	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &chn_cb_negotiate;
@@ -285,6 +288,8 @@ static void exit_hyperv_utils(void)
 	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback  	
&chn_cb_negotiate;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+	printk(KERN_INFO "De-Registered HyperV Utility Driver.\n");
 }
 
 module_init(init_hyperv_utils);
diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
index 7c07499..3291ab4 100644
--- a/drivers/staging/hv/utils.h
+++ b/drivers/staging/hv/utils.h
@@ -98,6 +98,10 @@ struct ictimesync_data{
 	u8 flags;
 } __attribute__((packed));
 
+
+/* Number of IC types supported */
+#define MAX_MSG_TYPES	3
+
 /* Index for each IC struct in array hv_cb_utils[] */
 #define HV_SHUTDOWN_MSG		0
 #define HV_TIMESYNC_MSG		1
@@ -115,5 +119,6 @@ extern void prep_negotiate_resp(struct icmsg_hdr *,
 				struct icmsg_negotiate *, u8 *);
 extern void chn_cb_negotiate(void *);
 extern struct hyperv_service_callback hv_cb_utils[];
+extern atomic_t hv_utils_initcnt;
 
 #endif /* __HV_UTILS_H_ */
-- 
1.6.3.2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0518-1-Fix-race-condition-in-hv_utils-module-initialization.patch
Type: application/octet-stream
Size: 4663 bytes
Desc: 0518-1-Fix-race-condition-in-hv_utils-module-initialization.patch
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20100519/29025f20/attachment.obj

Greg KH

2010-May-19 16:10 UTC

head link

[PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.

On Wed, May 19, 2010 at 03:54:17PM +0000, Haiyang Zhang
wrote:> From: Haiyang Zhang <haiyangz at microsoft.com>
> 
> Subject: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module
initialization.
> There is a possible race condition when hv_utils starts to load immediately
> after hv_vmbus is loading - null pointer error could happen.
> This patch added an atomic counter to ensure the hv_utils module
initialization
> happens after all vmbus IC channels are initialized.
> 
> Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen at microsoft.com>
> 
> ---
>  drivers/staging/hv/channel_mgmt.c |   26 +++++++++++++++-----------
>  drivers/staging/hv/hv_utils.c     |   11 ++++++++---
>  drivers/staging/hv/utils.h        |    5 +++++
>  3 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/hv/channel_mgmt.c
b/drivers/staging/hv/channel_mgmt.c
> index 3f53b4d..b5b6a70 100644
> --- a/drivers/staging/hv/channel_mgmt.c
> +++ b/drivers/staging/hv/channel_mgmt.c
> @@ -33,7 +33,6 @@ struct vmbus_channel_message_table_entry {
>  	void (*messageHandler)(struct vmbus_channel_message_header *msg);
>  };
>  
> -#define MAX_MSG_TYPES                    3
>  #define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
>  
>  static const struct hv_guid
> @@ -233,6 +232,11 @@ struct hyperv_service_callback
hv_cb_utils[MAX_MSG_TYPES] = {
>  };
>  EXPORT_SYMBOL(hv_cb_utils);
>  
> +/* Counter of IC channels initialized */
> +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
> +EXPORT_SYMBOL(hv_utils_initcnt);
> +
> +
>  /*
>   * AllocVmbusChannel - Allocate and initialize a vmbus channel object
>   */
> @@ -373,22 +377,22 @@ static void VmbusChannelProcessOffer(void *context)
>  		 * can cleanup properly
>  		 */
>  		newChannel->State = CHANNEL_OPEN_STATE;
> -		cnt = 0;
>  
> -		while (cnt != MAX_MSG_TYPES) {
> +		/* Open IC channels */
> +		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
>  			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
>  				   &hv_cb_utils[cnt].data,
> -				   sizeof(struct hv_guid)) == 0) {
> +				   sizeof(struct hv_guid)) == 0 &&
> +			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> +					     2 * PAGE_SIZE, NULL, 0,
> +					     hv_cb_utils[cnt].callback,
> +					     newChannel) == 0) {
> +				hv_cb_utils[cnt].channel = newChannel;
> +				mb();
>  				DPRINT_INFO(VMBUS, "%s",
>  					    hv_cb_utils[cnt].log_msg);
> -
> -				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> -						    2 * PAGE_SIZE, NULL, 0,
> -						    hv_cb_utils[cnt].callback,
> -						    newChannel) == 0)
> -					hv_cb_utils[cnt].channel = newChannel;
> +				atomic_inc(&hv_utils_initcnt);
>  			}
> -			cnt++;
>  		}
>  	}
>  	DPRINT_EXIT(VMBUS);
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 8a49aaf..f9826c7 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -253,7 +253,11 @@ static void heartbeat_onchannelcallback(void *context)
>  
>  static int __init init_hyperv_utils(void)
>  {
> -	printk(KERN_INFO "Registering HyperV Utility Driver\n");
> +	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
> +
> +	/* Wait until all IC channels are initialized */
> +	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
> +		msleep(100);
No, don't do this here, do something in your hv_vmbus core to handle
registering sub-drivers properly.  Perhaps you need to sleep there
before you can succeed on a initialization.
>  
>  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback >  	
&shutdown_onchannelcallback;
The problem is that you just have a bunch of callbacks you are setting
up, it's not a "real" function call.  Please change it over to a
function call, like all other subsystems have.  Then, you can handle any
"sleep until we are set up properly" issues in the vmbus code, not in
each and every individual bus driver.

thanks,

greg k-h

Reasonably Related Threads

Search for more seemingly similar threads

Virtualization - May 2010 - [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.

[PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.

[PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.

Reasonably Related Threads