Linux Virtualization - May 2010 - [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)

From: Haiyang Zhang <haiyangz at microsoft.com>

Subject: [PATCH] staging: hv: Fix race condition on IC channel 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 event waiting to ensure all channels are ready before
vmbus_init() returns. So another module won't have any uninitialized
channel.

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

---
 drivers/staging/hv/channel_mgmt.c  |   23 +++++++++++++----------
 drivers/staging/hv/vmbus_drv.c     |   10 ++++++++++
 drivers/staging/hv/vmbus_private.h |    1 +
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c
b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..f99db1b 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
 	int ret;
 	int cnt;
 	unsigned long flags;
+	static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
 
 	DPRINT_ENTER(VMBUS);
 
@@ -373,22 +374,24 @@ 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;
+				if (atomic_inc_return(&ic_channel_initcnt) =+				   MAX_MSG_TYPES)
+					osd_WaitEventSet(ic_channel_ready);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..3ae8981 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -989,6 +989,8 @@ static struct dmi_system_id __initdata
microsoft_hv_dmi_table[] = {
 };
 MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
 
+struct osd_waitevent *ic_channel_ready;
+
 static int __init vmbus_init(void)
 {
 	int ret = 0;
@@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
 	if (!dmi_check_system(microsoft_hv_dmi_table))
 		return -ENODEV;
 
+	ic_channel_ready = osd_WaitEventCreate();
+	if (ic_channel_ready == NULL)
+		return -ENOMEM;
+
 	ret = vmbus_bus_init(VmbusInitialize);
 
+	/* Wait until all IC channels are initialized */
+	osd_WaitEventWait(ic_channel_ready);
+
+	kfree(ic_channel_ready);
 	DPRINT_EXIT(VMBUS_DRV);
 	return ret;
 }
diff --git a/drivers/staging/hv/vmbus_private.h
b/drivers/staging/hv/vmbus_private.h
index 588c667..3fb8dad 100644
--- a/drivers/staging/hv/vmbus_private.h
+++ b/drivers/staging/hv/vmbus_private.h
@@ -130,5 +130,6 @@ int VmbusSetEvent(u32 childRelId);
 
 void VmbusOnEvents(void);
 
+extern struct osd_waitevent *ic_channel_ready;
 
 #endif /* _VMBUS_PRIVATE_H_ */
-- 
1.6.3.2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0525-Fix-race-condition-on-IC-channel-initialization.patch
Type: application/octet-stream
Size: 3454 bytes
Desc: 0525-Fix-race-condition-on-IC-channel-initialization.patch
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20100526/55614c19/attachment.obj

On Wed, May 26, 2010 at 04:54:39PM +0000, Haiyang Zhang
wrote:
> From: Haiyang Zhang <haiyangz at microsoft.com>
> 
> Subject: [PATCH] staging: hv: Fix race condition on IC channel
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 event waiting to ensure all channels are ready before
> vmbus_init() returns. So another module won't have any uninitialized
channel.
> 
> Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen at microsoft.com>
> 
> ---
>  drivers/staging/hv/channel_mgmt.c  |   23 +++++++++++++----------
>  drivers/staging/hv/vmbus_drv.c     |   10 ++++++++++
>  drivers/staging/hv/vmbus_private.h |    1 +
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/hv/channel_mgmt.c
b/drivers/staging/hv/channel_mgmt.c
> index 3f53b4d..f99db1b 100644
> --- a/drivers/staging/hv/channel_mgmt.c
> +++ b/drivers/staging/hv/channel_mgmt.c
> @@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
>  	int ret;
>  	int cnt;
>  	unsigned long flags;
> +	static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
Why is this an atomic_t?
>  	DPRINT_ENTER(VMBUS);
>  
> @@ -373,22 +374,24 @@ 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();
What is the mb() call for?  Why is it necessary?  (hint, if you need it,
something else is really wrong...)

Something wierd happened with your indentation here, it doesn't line up
properly.  That call to VmbusChannelOpen() needs to go in a full tab,
not 4 spaces.

Please always run your patch through the checkpatch.pl script before
sending it to me.

>  				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;
> +				if (atomic_inc_return(&ic_channel_initcnt) => +				  
MAX_MSG_TYPES)
> +					osd_WaitEventSet(ic_channel_ready);
>  			}
> -			cnt++;
>  		}
>  	}
>  	DPRINT_EXIT(VMBUS);
> diff --git a/drivers/staging/hv/vmbus_drv.c
b/drivers/staging/hv/vmbus_drv.c
> index c21731a..3ae8981 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -989,6 +989,8 @@ static struct dmi_system_id __initdata
microsoft_hv_dmi_table[] = {
>  };
>  MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
>  
> +struct osd_waitevent *ic_channel_ready;
What's with the "ic_" naming scheme here?  It should be
"hv_" right?
> +
>  static int __init vmbus_init(void)
>  {
>  	int ret = 0;
> @@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
>  	if (!dmi_check_system(microsoft_hv_dmi_table))
>  		return -ENODEV;
>  
> +	ic_channel_ready = osd_WaitEventCreate();
> +	if (ic_channel_ready == NULL)
> +		return -ENOMEM;
> +
>  	ret = vmbus_bus_init(VmbusInitialize);
As you are using this "ic_channel_ready variable only within the
vmbus_bus_init() call, why not just make it local to there?  Then
there's no need to do the create/init/wait/free sequence outside the
init call.

The init call should just do all of this for us, right?

thanks,

greg k-h