RE: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiationcode for util services

[KY Srinivasan]

> -----Original Message-----
> From: K. Y. Srinivasan [mailto:kys@xxxxxxxxxxxxx]
> Sent: Tuesday, July 02, 2013 1:32 PM
> To: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> jasowang@xxxxxxxxxx
> Cc: KY Srinivasan
> Subject: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for util
> services
> 
> The current code picked the highest version advertised by the host. WS2012 R2
> has implemented a protocol version for KVP that is not compatible with prior
> protocol versions of KVP. Fix the bug in the current code by explicitly specifying
> the protocol version that the guest can support.

Greg,

Do you want me to resubmit this patch.

Regards,

K. Y
> 
> Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> ---
>  drivers/hv/channel_mgmt.c |   75 +++++++++++++++++++++++++++++++-------
> ------
>  drivers/hv/hv_kvp.c       |   24 ++++++++++++++-
>  drivers/hv/hv_snapshot.c  |   18 +++-------
>  drivers/hv/hv_util.c      |   21 +++++++++++--
>  include/linux/hyperv.h    |   10 +++++-
>  5 files changed, 109 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 0df7590..12ec8c8 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -48,30 +48,39 @@ struct vmbus_channel_message_table_entry {
>   * @negop is of type &struct icmsg_negotiate.
>   * Set up and fill in default negotiate response message.
>   *
> - * The max_fw_version specifies the maximum framework version that
> - * we can support and max _srv_version specifies the maximum service
> - * version we can support. A special value MAX_SRV_VER can be
> - * specified to indicate that we can handle the maximum version
> - * exposed by the host.
> + * The fw_version specifies the  framework version that
> + * we can support and srv_version specifies the service
> + * version we can support.
>   *
>   * Mainly used by Hyper-V drivers.
>   */
> -void vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> +bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
>  				struct icmsg_negotiate *negop, u8 *buf,
> -				int max_fw_version, int max_srv_version)
> +				int fw_version, int srv_version)
>  {
> -	int icframe_vercnt;
> -	int icmsg_vercnt;
> +	int icframe_major, icframe_minor;
> +	int icmsg_major, icmsg_minor;
> +	int fw_major, fw_minor;
> +	int srv_major, srv_minor;
>  	int i;
> +	bool found_match = false;
> 
>  	icmsghdrp->icmsgsize = 0x10;
> +	fw_major = (fw_version >> 16);
> +	fw_minor = (fw_version & 0xFFFF);
> +
> +	srv_major = (srv_version >> 16);
> +	srv_minor = (srv_version & 0xFFFF);
> 
>  	negop = (struct icmsg_negotiate *)&buf[
>  		sizeof(struct vmbuspipe_hdr) +
>  		sizeof(struct icmsg_hdr)];
> 
> -	icframe_vercnt = negop->icframe_vercnt;
> -	icmsg_vercnt = negop->icmsg_vercnt;
> +	icframe_major = negop->icframe_vercnt;
> +	icframe_minor = 0;
> +
> +	icmsg_major = negop->icmsg_vercnt;
> +	icmsg_minor = 0;
> 
>  	/*
>  	 * Select the framework version number we will
> @@ -79,26 +88,48 @@ void vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
>  	 */
> 
>  	for (i = 0; i < negop->icframe_vercnt; i++) {
> -		if (negop->icversion_data[i].major <= max_fw_version)
> -			icframe_vercnt = negop->icversion_data[i].major;
> +		if ((negop->icversion_data[i].major == fw_major) &&
> +		   (negop->icversion_data[i].minor == fw_minor)) {
> +			icframe_major = negop->icversion_data[i].major;
> +			icframe_minor = negop->icversion_data[i].minor;
> +			found_match = true;
> +		}
>  	}
> 
> +	if (!found_match)
> +		goto fw_error;
> +
> +	found_match = false;
> +
>  	for (i = negop->icframe_vercnt;
>  		 (i < negop->icframe_vercnt + negop->icmsg_vercnt); i++) {
> -		if (negop->icversion_data[i].major <= max_srv_version)
> -			icmsg_vercnt = negop->icversion_data[i].major;
> +		if ((negop->icversion_data[i].major == srv_major) &&
> +		   (negop->icversion_data[i].minor == srv_minor)) {
> +			icmsg_major = negop->icversion_data[i].major;
> +			icmsg_minor = negop->icversion_data[i].minor;
> +			found_match = true;
> +		}
>  	}
> 
>  	/*
> -	 * Respond with the maximum framework and service
> +	 * Respond with the framework and service
>  	 * version numbers we can support.
>  	 */
> -	negop->icframe_vercnt = 1;
> -	negop->icmsg_vercnt = 1;
> -	negop->icversion_data[0].major = icframe_vercnt;
> -	negop->icversion_data[0].minor = 0;
> -	negop->icversion_data[1].major = icmsg_vercnt;
> -	negop->icversion_data[1].minor = 0;
> +
> +fw_error:
> +	if (!found_match) {
> +		negop->icframe_vercnt = 0;
> +		negop->icmsg_vercnt = 0;
> +	} else {
> +		negop->icframe_vercnt = 1;
> +		negop->icmsg_vercnt = 1;
> +	}
> +
> +	negop->icversion_data[0].major = icframe_major;
> +	negop->icversion_data[0].minor = icframe_minor;
> +	negop->icversion_data[1].major = icmsg_major;
> +	negop->icversion_data[1].minor = icmsg_minor;
> +	return found_match;
>  }
> 
>  EXPORT_SYMBOL_GPL(vmbus_prep_negotiate_resp);
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index ed50e9e..5312720 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -29,6 +29,16 @@
>  #include <linux/hyperv.h>
> 
> 
> +/*
> + * Pre win8 version numbers used in ws2008 and ws 2008 r2 (win7)
> + */
> +#define WIN7_SRV_MAJOR   3
> +#define WIN7_SRV_MINOR   0
> +#define WIN7_SRV_MAJOR_MINOR     (WIN7_SRV_MAJOR << 16 |
> WIN7_SRV_MINOR)
> +
> +#define WIN8_SRV_MAJOR   4
> +#define WIN8_SRV_MINOR   0
> +#define WIN8_SRV_MAJOR_MINOR     (WIN8_SRV_MAJOR << 16 |
> WIN8_SRV_MINOR)
> 
>  /*
>   * Global state maintained for transaction that is being processed.
> @@ -593,8 +603,19 @@ void hv_kvp_onchannelcallback(void *context)
>  			sizeof(struct vmbuspipe_hdr)];
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> +			/*
> +			 * We start with win8 version and if the host cannot
> +			 * support that we use the previous version.
> +			 */
> +			if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
> +				 recv_buffer, UTIL_FW_MAJOR_MINOR,
> +				 WIN8_SRV_MAJOR_MINOR))
> +				goto done;
> +
>  			vmbus_prep_negotiate_resp(icmsghdrp, negop,
> -				 recv_buffer, MAX_SRV_VER, MAX_SRV_VER);
> +				 recv_buffer, UTIL_FW_MAJOR_MINOR,
> +				 WIN7_SRV_MAJOR_MINOR);
> +
>  		} else {
>  			kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
>  				sizeof(struct vmbuspipe_hdr) +
> @@ -626,6 +647,7 @@ void hv_kvp_onchannelcallback(void *context)
>  			return;
> 
>  		}
> +done:
> 
>  		icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
>  			| ICMSGHDRFLAG_RESPONSE;
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index 8ad5653..e4572f3 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -24,6 +24,10 @@
>  #include <linux/workqueue.h>
>  #include <linux/hyperv.h>
> 
> +#define VSS_MAJOR  5
> +#define VSS_MINOR  0
> +#define VSS_MAJOR_MINOR    (VSS_MAJOR << 16 | VSS_MINOR)
> +
> 
> 
>  /*
> @@ -186,18 +190,8 @@ void hv_vss_onchannelcallback(void *context)
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
>  			vmbus_prep_negotiate_resp(icmsghdrp, negop,
> -				 recv_buffer, MAX_SRV_VER, MAX_SRV_VER);
> -			/*
> -			 * We currently negotiate the highest number the
> -			 * host has presented. If this version is not
> -			 * atleast 5.0, reject.
> -			 */
> -			negop = (struct icmsg_negotiate *)&recv_buffer[
> -				sizeof(struct vmbuspipe_hdr) +
> -				sizeof(struct icmsg_hdr)];
> -
> -			if (negop->icversion_data[1].major < 5)
> -				negop->icframe_vercnt = 0;
> +				 recv_buffer, UTIL_FW_MAJOR_MINOR,
> +				 VSS_MAJOR_MINOR);
>  		} else {
>  			vss_msg = (struct hv_vss_msg *)&recv_buffer[
>  				sizeof(struct vmbuspipe_hdr) +
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 2f561c5..c16164d 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -28,6 +28,18 @@
>  #include <linux/reboot.h>
>  #include <linux/hyperv.h>
> 
> +#define SHUTDOWN_MAJOR	3
> +#define SHUTDOWN_MINOR  0
> +#define SHUTDOWN_MAJOR_MINOR	(SHUTDOWN_MAJOR << 16 |
> SHUTDOWN_MINOR)
> +
> +#define TIMESYNCH_MAJOR	3
> +#define TIMESYNCH_MINOR 0
> +#define TIMESYNCH_MAJOR_MINOR	(TIMESYNCH_MAJOR << 16 |
> TIMESYNCH_MINOR)
> +
> +#define HEARTBEAT_MAJOR	3
> +#define HEARTBEAT_MINOR 0
> +#define HEARTBEAT_MAJOR_MINOR	(HEARTBEAT_MAJOR << 16 |
> HEARTBEAT_MINOR)
> +
>  static void shutdown_onchannelcallback(void *context);
>  static struct hv_util_service util_shutdown = {
>  	.util_cb = shutdown_onchannelcallback,
> @@ -87,7 +99,8 @@ static void shutdown_onchannelcallback(void *context)
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
>  			vmbus_prep_negotiate_resp(icmsghdrp, negop,
> -					shut_txf_buf, MAX_SRV_VER,
> MAX_SRV_VER);
> +					shut_txf_buf, UTIL_FW_MAJOR_MINOR,
> +					SHUTDOWN_MAJOR_MINOR);
>  		} else {
>  			shutdown_msg =
>  				(struct shutdown_msg_data *)&shut_txf_buf[
> @@ -213,7 +226,8 @@ static void timesync_onchannelcallback(void *context)
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
>  			vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> time_txf_buf,
> -						MAX_SRV_VER, MAX_SRV_VER);
> +						UTIL_FW_MAJOR_MINOR,
> +						TIMESYNCH_MAJOR_MINOR);
>  		} else {
>  			timedatap = (struct ictimesync_data *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr) +
> @@ -253,7 +267,8 @@ static void heartbeat_onchannelcallback(void *context)
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
>  			vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> -				hbeat_txf_buf, MAX_SRV_VER, MAX_SRV_VER);
> +				hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
> +				HEARTBEAT_MAJOR_MINOR);
>  		} else {
>  			heartbeat_msg =
>  				(struct heartbeat_msg_data *)&hbeat_txf_buf[
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index fae8bac..4994907 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -27,6 +27,14 @@
> 
>  #include <linux/types.h>
> 
> +/*
> + * Framework version for util services.
> + */
> +
> +#define UTIL_FW_MAJOR  3
> +#define UTIL_FW_MINOR  0
> +#define UTIL_FW_MAJOR_MINOR     (UTIL_FW_MAJOR << 16 |
> UTIL_FW_MINOR)
> +
> 
>  /*
>   * Implementation of host controlled snapshot of the guest.
> @@ -1494,7 +1502,7 @@ struct hyperv_service_callback {
>  };
> 
>  #define MAX_SRV_VER	0x7ffffff
> -extern void vmbus_prep_negotiate_resp(struct icmsg_hdr *,
> +extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
>  					struct icmsg_negotiate *, u8 *, int,
>  					int);
> 
> --
> 1.7.4.1
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/