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

[Haiyang Zhang]

[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]

From: Haiyang Zhang <haiyangz@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@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@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


[-- Attachment #2: 0518-1-Fix-race-condition-in-hv_utils-module-initialization.patch --]
[-- Type: application/octet-stream, Size: 4663 bytes --]

From: Haiyang Zhang <haiyangz@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@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@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


[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

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

[Greg KH]

On Wed, May 19, 2010 at 03:54:17PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@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@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@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

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

[Haiyang Zhang]

> > +	/* 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.

Thanks for the recommendation. I will put the sleep into vmbus_init to 
ensure all channels are ready before the vmbus_init function exits.

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

Actually, we already assign a default callback function, chn_cb_negotiate(),
when the channels are opened in vmbus module. It's a real function and can 
handle common negotiation messages.
I will move the sleep into vmbus module as well.

Thanks,

- Haiyang

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

[Greg KH]

On Wed, May 19, 2010 at 08:30:25PM +0000, Haiyang Zhang wrote:
> > > +	/* 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.
> 
> Thanks for the recommendation. I will put the sleep into vmbus_init to 
> ensure all channels are ready before the vmbus_init function exits.
> 
> > >  	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.
> 
> Actually, we already assign a default callback function, chn_cb_negotiate(),
> when the channels are opened in vmbus module. It's a real function and can 
> handle common negotiation messages.

Then why don't you use it here?

> I will move the sleep into vmbus module as well.

I still think there's a real problem somewhere else in the architecture
if such a sleep is necessary...

Is the issue that the modprobe of the hv_vmbus can return before the bus
is really all set up and ready to go?  If so, just fix that, then you
will not need any "sleep" calls anywhere, right?

thanks,

greg k-h

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

[Haiyang Zhang]

> > Actually, we already assign a default callback function,
> chn_cb_negotiate(),
> > when the channels are opened in vmbus module. It's a real function
> and can
> > handle common negotiation messages.
> 
> Then why don't you use it here?

When vmbus is loaded and channel is offered from HyperV host, the default
callback function, chn_cb_negotiate(), is assigned to the function ptr, and
used to do basic responses of negotiation messages. 

After hv_utils modules is loaded the callback function ptr is overridden by 
a specialized function in hv_utils module, and handles each feature (shutdown, 
timesync, etc.) differently.

> I still think there's a real problem somewhere else in the architecture
> if such a sleep is necessary...
> 
> Is the issue that the modprobe of the hv_vmbus can return before the
> bus
> is really all set up and ready to go?  If so, just fix that, then you
> will not need any "sleep" calls anywhere, right?

After vmbus is loaded, the channel offering will come from the host, then
it initializes the channel. The channel offering can happen a little later
after vmbus_init() is done and modprobe returns. So I think we should let
vmbus_init function wait(sleep) until all channel offerings are received before
returning. This will ensure all channels are ready before modprobe returns.

Thanks,

- Haiyang

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

[Greg KH]

On Wed, May 19, 2010 at 10:12:51PM +0000, Haiyang Zhang wrote:
> > > Actually, we already assign a default callback function,
> > chn_cb_negotiate(),
> > > when the channels are opened in vmbus module. It's a real function
> > and can
> > > handle common negotiation messages.
> > 
> > Then why don't you use it here?
> 
> When vmbus is loaded and channel is offered from HyperV host, the default
> callback function, chn_cb_negotiate(), is assigned to the function ptr, and
> used to do basic responses of negotiation messages. 

Great, so that works.

> After hv_utils modules is loaded the callback function ptr is overridden by 
> a specialized function in hv_utils module, and handles each feature (shutdown, 
> timesync, etc.) differently.

That's the problem.  Provide a "correct" interface to properly change
the callback function.  Just setting function pointers in a random
manner is ripe for all sorts of bad problems, don't you agree?  Heck, I
don't see any locking happening here which could cause messages to be
handled when things are only half-way set up.  Also, what's to say your
function pointer write is atomic in the first place :)

In short, use proper locking for something like this.

> > I still think there's a real problem somewhere else in the architecture
> > if such a sleep is necessary...
> > 
> > Is the issue that the modprobe of the hv_vmbus can return before the
> > bus
> > is really all set up and ready to go?  If so, just fix that, then you
> > will not need any "sleep" calls anywhere, right?
> 
> After vmbus is loaded, the channel offering will come from the host, then
> it initializes the channel. The channel offering can happen a little later
> after vmbus_init() is done and modprobe returns. So I think we should let
> vmbus_init function wait(sleep) until all channel offerings are received before
> returning. This will ensure all channels are ready before modprobe returns.

That sounds like a good idea.

thanks,

greg k-h