linux-kernel - Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 26 May 2010 13:51:34 -0700
From:	Greg KH <gregkh@...e.de>
To:	Haiyang Zhang <haiyangz@...rosoft.com>
Cc:	"'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
	"'devel@...verdev.osuosl.org'" <devel@...verdev.osuosl.org>,
	"'virtualization@...ts.osdl.org'" <virtualization@...ts.osdl.org>,
	Hank Janssen <hjanssen@...rosoft.com>
Subject: Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel
 initialization (modified)

On Wed, May 26, 2010 at 04:54:39PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@...rosoft.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@...rosoft.com>
> Signed-off-by: Hank Janssen <hjanssen@...rosoft.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/