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

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


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

[ message continues ]

As discussed previously, I used atomic_t to handle more general case 

It ensures the channel assignment happens before the wakeup call: 
osd_WaitEventSet(ic_channel_ready), if the compiler optimization re-arrange 

Sure, I will replace it with TAB. I already ran checkpatch.pl on 
this patch -- no error:
staging-next-2.6> scripts/checkpatch.pl 0525-Fix-race-condition-on-IC-channel-initialization.patch
total: 0 errors, 0 warnings, 71 lines checked


IC stands for "integration components", such as Shutdown, Timesync, 

The ic_channel_ready variable is called by VmbusChannelProcessOffer /  osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's 
not a local variable.

Thanks,

- Haiyang

--

[ message continues ]

If you care about this, because some other thread is looking at it, then
you really need to protect it with a lock.  Don't rely on a mb() to get



But again, this logic should be within the init call, as it's part of
the proper init sequence.  So just put it in that call please.

thanks,

greg k-h
--

[ message continues ]

Marketing in the their infinite wisdom decided that going forward they 
should be called Linux Integration Service so IS. 

Sigh..............

We will change it to hv_  :)

Hank.

--

[ message continues ]

The VmbusChannelProcessOffer() is called from interrupt context, and initialize the channels, wake up vmbus_init when all channels are ready. If using local variable only, how to pass the channel ready info to vmbus_init() which is in a different context?

Thanks,

- Haiyang

--

[ message continues ]

How about a lock!

What's so scary about a pretty little semaphore?  They are all cute and
cuddley and don't bite anyone.  You should not be afraid to use them,

No, I mean move the logic you added here, into the vmbus_init() call.

thanks,

greg k-h
--

[ message continues ]

Do you mean:
Move the event creat/wait/free, which is currently in vmbus_init(), into vmbus_bus_init() function.  hv_channle_ready will still be a global variable. And, the wakeup call -- osd_WaitEventSet() --remains in VmbusChannelProcessOffer() ?

Thanks,

- Haiyang

--

[ message continues ]

Yes.

And as Jiri pointed out, this should be a simple completion.

thanks,

greg k-h
--

[ message continues ]

wake_up() is a barrier, you don't need the mb() there.

BTW osd_WaitEventSet et al. can be easily converted to completion.

-- 
js
--

[ message continues ]