[PATCH 0/4] hv: utils: propagate state to interrupt thread

[PATCH 0/4] hv: utils: propagate state to interrupt thread

[Olaf Hering]

The Copy-VMFile cmdlet on the host may fail because the guest fcopy
driver state machine gets out of sync. This happens because the ->state
and ->context variables are accessed by the main thread and from
interrupt context. If an interrupt happens between fcopy_respond_to_host
and hv_poll_channel in fcopy_write, then hv_fcopy_onchannelcallback
called from that interrupt sees still state HVUTIL_USERSPACE_RECV. It
updates the context, but fcopy_write will not notice that update and
hv_poll_channel gets called with an empty context.  As a result
hv_fcopy_daemon gets no more data. After a timeout Copy-VMFile fails
with timeout.

In my initial testing for a fix I put a "mb()" after the last .state
change in fcopy_write. But this series implementes read/write memory
barriers as needed. Let me know if this is overdoing things.

Olaf

Olaf Hering (4):
  hv: add helpers to handle hv_util device state
  hv: fcopy: use wrapper to propate state
  hv: kvp: use wrapper to propate state
  hv: vss: use wrapper to propate state

 drivers/hv/hv_fcopy.c     | 36 ++++++++++++++++++++----------------
 drivers/hv/hv_kvp.c       | 38 +++++++++++++++++++++-----------------
 drivers/hv/hv_snapshot.c  | 37 ++++++++++++++++++++-----------------
 drivers/hv/hyperv_vmbus.h | 12 ++++++++++++
 4 files changed, 73 insertions(+), 50 deletions(-)

[PATCH 1/4] hv: add helpers to handle hv_util device state

[Olaf Hering]

The callbacks in kvp, vss and fcopy code are called the main thread and
also from interrupt context. If a state change is done by the main
thread it is not immediately seen by the interrupt. As a result the
state machine gets out of sync.

Force propagation of state changes via get/set helpers with a memory barrier.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/hv/hyperv_vmbus.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 3d70e36..6c03925 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -780,4 +780,16 @@ enum hvutil_device_state {
 	HVUTIL_DEVICE_DYING,     /* driver unload is in progress */
 };
 
+static inline void hvutil_device_set_state(enum hvutil_device_state *p, enum hvutil_device_state s)
+{
+	*p = s;
+	wmb();
+}
+
+static inline enum hvutil_device_state hvutil_device_get_state(enum hvutil_device_state *p)
+{
+       rmb();
+       return *p;
+}
+
 #endif /* _HYPERV_VMBUS_H */

[PATCH 2/4] hv: fcopy: use wrapper to propate state

[Olaf Hering]

The .state is used by several threads of execution.
Propagate the state to make changes visible. Also propagate context
change in hv_fcopy_onchannelcallback.
Without this change fcopy may hang at random points.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/hv/hv_fcopy.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index db4b887..47d9c34 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -46,7 +46,7 @@
  */
 
 static struct {
-	int state;   /* hvutil_device_state */
+	enum hvutil_device_state state;
 	int recv_len; /* number of bytes received. */
 	struct hv_fcopy_hdr  *fcopy_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
@@ -67,6 +67,9 @@ static struct hvutil_transport *hvt;
  */
 static int dm_reg_value;
 
+#define fcopy_get_state() hvutil_device_get_state(&fcopy_transaction.state)
+#define fcopy_set_state(s) hvutil_device_set_state(&fcopy_transaction.state, s)
+
 static void fcopy_timeout_func(struct work_struct *dummy)
 {
 	/*
@@ -76,8 +79,8 @@ static void fcopy_timeout_func(struct work_struct *dummy)
 	fcopy_respond_to_host(HV_E_FAIL);
 
 	/* Transaction is finished, reset the state. */
-	if (fcopy_transaction.state > HVUTIL_READY)
-		fcopy_transaction.state = HVUTIL_READY;
+	if (fcopy_get_state() > HVUTIL_READY)
+		fcopy_set_state(HVUTIL_READY);
 
 	hv_poll_channel(fcopy_transaction.fcopy_context,
 			hv_fcopy_onchannelcallback);
@@ -108,7 +111,7 @@ static int fcopy_handle_handshake(u32 version)
 		return -EINVAL;
 	}
 	pr_debug("FCP: userspace daemon ver. %d registered\n", version);
-	fcopy_transaction.state = HVUTIL_READY;
+	fcopy_set_state(HVUTIL_READY);
 	hv_poll_channel(fcopy_transaction.fcopy_context,
 			hv_fcopy_onchannelcallback);
 	return 0;
@@ -162,13 +165,13 @@ static void fcopy_send_data(struct work_struct *dummy)
 		break;
 	}
 
-	fcopy_transaction.state = HVUTIL_USERSPACE_REQ;
+	fcopy_set_state(HVUTIL_USERSPACE_REQ);
 	rc = hvutil_transport_send(hvt, out_src, out_len);
 	if (rc) {
 		pr_debug("FCP: failed to communicate to the daemon: %d\n", rc);
 		if (cancel_delayed_work_sync(&fcopy_timeout_work)) {
 			fcopy_respond_to_host(HV_E_FAIL);
-			fcopy_transaction.state = HVUTIL_READY;
+			fcopy_set_state(HVUTIL_READY);
 		}
 	}
 	kfree(smsg_out);
@@ -227,12 +230,13 @@ void hv_fcopy_onchannelcallback(void *context)
 	int util_fw_version;
 	int fcopy_srv_version;
 
-	if (fcopy_transaction.state > HVUTIL_READY) {
+	if (fcopy_get_state() > HVUTIL_READY) {
 		/*
 		 * We will defer processing this callback once
 		 * the current transaction is complete.
 		 */
 		fcopy_transaction.fcopy_context = context;
+		wmb();
 		return;
 	}
 	fcopy_transaction.fcopy_context = NULL;
@@ -264,12 +268,12 @@ void hv_fcopy_onchannelcallback(void *context)
 		fcopy_transaction.recv_req_id = requestid;
 		fcopy_transaction.fcopy_msg = fcopy_msg;
 
-		if (fcopy_transaction.state < HVUTIL_READY) {
+		if (fcopy_get_state() < HVUTIL_READY) {
 			/* Userspace is not registered yet */
 			fcopy_respond_to_host(HV_E_FAIL);
 			return;
 		}
-		fcopy_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
+		fcopy_set_state(HVUTIL_HOSTMSG_RECEIVED);
 
 		/*
 		 * Send the information to the user-level daemon.
@@ -291,10 +295,10 @@ static int fcopy_on_msg(void *msg, int len)
 	if (len != sizeof(int))
 		return -EINVAL;
 
-	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
+	if (fcopy_get_state() == HVUTIL_DEVICE_INIT)
 		return fcopy_handle_handshake(*val);
 
-	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
+	if (fcopy_get_state() != HVUTIL_USERSPACE_REQ)
 		return -EINVAL;
 
 	/*
@@ -302,9 +306,9 @@ static int fcopy_on_msg(void *msg, int len)
 	 * to the host. But first, cancel the timeout.
 	 */
 	if (cancel_delayed_work_sync(&fcopy_timeout_work)) {
-		fcopy_transaction.state = HVUTIL_USERSPACE_RECV;
+		fcopy_set_state(HVUTIL_USERSPACE_RECV);
 		fcopy_respond_to_host(*val);
-		fcopy_transaction.state = HVUTIL_READY;
+		fcopy_set_state(HVUTIL_READY);
 		hv_poll_channel(fcopy_transaction.fcopy_context,
 				hv_fcopy_onchannelcallback);
 	}
@@ -317,7 +321,7 @@ static void fcopy_on_reset(void)
 	/*
 	 * The daemon has exited; reset the state.
 	 */
-	fcopy_transaction.state = HVUTIL_DEVICE_INIT;
+	fcopy_set_state(HVUTIL_DEVICE_INIT);
 
 	if (cancel_delayed_work_sync(&fcopy_timeout_work))
 		fcopy_respond_to_host(HV_E_FAIL);
@@ -333,7 +337,7 @@ int hv_fcopy_init(struct hv_util_service *srv)
 	 * Defer processing channel callbacks until the daemon
 	 * has registered.
 	 */
-	fcopy_transaction.state = HVUTIL_DEVICE_INIT;
+	fcopy_set_state(HVUTIL_DEVICE_INIT);
 
 	hvt = hvutil_transport_init(fcopy_devname, 0, 0,
 				    fcopy_on_msg, fcopy_on_reset);
@@ -345,7 +349,7 @@ int hv_fcopy_init(struct hv_util_service *srv)
 
 void hv_fcopy_deinit(void)
 {
-	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
+	fcopy_set_state(HVUTIL_DEVICE_DYING);
 	cancel_delayed_work_sync(&fcopy_timeout_work);
 	hvutil_transport_destroy(hvt);
 }

[PATCH 3/4] hv: kvp: use wrapper to propate state

[Olaf Hering]

The .state is used by several threads of execution.
Propagate the state to make changes visible. Also propagate context
change in kvp_on_msg.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/hv/hv_kvp.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index 74c38a9..f421691 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -61,7 +61,7 @@
  */
 
 static struct {
-	int state;   /* hvutil_device_state */
+	enum hvutil_device_state state;
 	int recv_len; /* number of bytes received. */
 	struct hv_kvp_msg  *kvp_msg; /* current message */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
@@ -74,6 +74,9 @@ static struct {
  */
 static int dm_reg_value;
 
+#define kvp_get_state() hvutil_device_get_state(&kvp_transaction.state)
+#define kvp_set_state(s) hvutil_device_set_state(&kvp_transaction.state, s)
+
 static void kvp_send_key(struct work_struct *dummy);
 
 
@@ -122,8 +125,8 @@ static void kvp_timeout_func(struct work_struct *dummy)
 	kvp_respond_to_host(NULL, HV_E_FAIL);
 
 	/* Transaction is finished, reset the state. */
-	if (kvp_transaction.state > HVUTIL_READY)
-		kvp_transaction.state = HVUTIL_READY;
+	if (kvp_get_state() > HVUTIL_READY)
+		kvp_set_state(HVUTIL_READY);
 
 	hv_poll_channel(kvp_transaction.kvp_context,
 			hv_kvp_onchannelcallback);
@@ -153,7 +156,7 @@ static int kvp_handle_handshake(struct hv_kvp_msg *msg)
 	pr_debug("KVP: userspace daemon ver. %d registered\n",
 		 KVP_OP_REGISTER);
 	kvp_register(dm_reg_value);
-	kvp_transaction.state = HVUTIL_READY;
+	kvp_set_state(HVUTIL_READY);
 
 	return 0;
 }
@@ -177,15 +180,15 @@ static int kvp_on_msg(void *msg, int len)
 	 * with the daemon; handle that first.
 	 */
 
-	if (kvp_transaction.state < HVUTIL_READY) {
+	if (kvp_get_state() < HVUTIL_READY) {
 		return kvp_handle_handshake(message);
 	}
 
 	/* We didn't send anything to userspace so the reply is spurious */
-	if (kvp_transaction.state < HVUTIL_USERSPACE_REQ)
+	if (kvp_get_state() < HVUTIL_USERSPACE_REQ)
 		return -EINVAL;
 
-	kvp_transaction.state = HVUTIL_USERSPACE_RECV;
+	kvp_set_state(HVUTIL_USERSPACE_RECV);
 
 	/*
 	 * Based on the version of the daemon, we propagate errors from the
@@ -218,7 +221,7 @@ static int kvp_on_msg(void *msg, int len)
 	 */
 	if (cancel_delayed_work_sync(&kvp_timeout_work)) {
 		kvp_respond_to_host(message, error);
-		kvp_transaction.state = HVUTIL_READY;
+		kvp_set_state(HVUTIL_READY);
 		hv_poll_channel(kvp_transaction.kvp_context,
 				hv_kvp_onchannelcallback);
 	}
@@ -349,7 +352,7 @@ kvp_send_key(struct work_struct *dummy)
 	int rc;
 
 	/* The transaction state is wrong. */
-	if (kvp_transaction.state != HVUTIL_HOSTMSG_RECEIVED)
+	if (kvp_get_state() != HVUTIL_HOSTMSG_RECEIVED)
 		return;
 
 	message = kzalloc(sizeof(*message), GFP_KERNEL);
@@ -442,13 +445,13 @@ kvp_send_key(struct work_struct *dummy)
 			break;
 	}
 
-	kvp_transaction.state = HVUTIL_USERSPACE_REQ;
+	kvp_set_state(HVUTIL_USERSPACE_REQ);
 	rc = hvutil_transport_send(hvt, message, sizeof(*message));
 	if (rc) {
 		pr_debug("KVP: failed to communicate to the daemon: %d\n", rc);
 		if (cancel_delayed_work_sync(&kvp_timeout_work)) {
 			kvp_respond_to_host(message, HV_E_FAIL);
-			kvp_transaction.state = HVUTIL_READY;
+			kvp_set_state(HVUTIL_READY);
 		}
 	}
 
@@ -596,12 +599,13 @@ void hv_kvp_onchannelcallback(void *context)
 	int util_fw_version;
 	int kvp_srv_version;
 
-	if (kvp_transaction.state > HVUTIL_READY) {
+	if (kvp_get_state() > HVUTIL_READY) {
 		/*
 		 * We will defer processing this callback once
 		 * the current transaction is complete.
 		 */
 		kvp_transaction.kvp_context = context;
+		wmb();
 		return;
 	}
 	kvp_transaction.kvp_context = NULL;
@@ -651,12 +655,12 @@ void hv_kvp_onchannelcallback(void *context)
 			kvp_transaction.recv_req_id = requestid;
 			kvp_transaction.kvp_msg = kvp_msg;
 
-			if (kvp_transaction.state < HVUTIL_READY) {
+			if (kvp_get_state() < HVUTIL_READY) {
 				/* Userspace is not registered yet */
 				kvp_respond_to_host(NULL, HV_E_FAIL);
 				return;
 			}
-			kvp_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
+			kvp_set_state(HVUTIL_HOSTMSG_RECEIVED);
 
 			/*
 			 * Get the information from the
@@ -688,7 +692,7 @@ static void kvp_on_reset(void)
 {
 	if (cancel_delayed_work_sync(&kvp_timeout_work))
 		kvp_respond_to_host(NULL, HV_E_FAIL);
-	kvp_transaction.state = HVUTIL_DEVICE_INIT;
+	kvp_set_state(HVUTIL_DEVICE_INIT);
 }
 
 int
@@ -702,7 +706,7 @@ hv_kvp_init(struct hv_util_service *srv)
 	 * Defer processing channel callbacks until the daemon
 	 * has registered.
 	 */
-	kvp_transaction.state = HVUTIL_DEVICE_INIT;
+	kvp_set_state(HVUTIL_DEVICE_INIT);
 
 	hvt = hvutil_transport_init(kvp_devname, CN_KVP_IDX, CN_KVP_VAL,
 				    kvp_on_msg, kvp_on_reset);
@@ -714,7 +718,7 @@ hv_kvp_init(struct hv_util_service *srv)
 
 void hv_kvp_deinit(void)
 {
-	kvp_transaction.state = HVUTIL_DEVICE_DYING;
+	kvp_set_state(HVUTIL_DEVICE_DYING);
 	cancel_delayed_work_sync(&kvp_timeout_work);
 	cancel_work_sync(&kvp_sendkey_work);
 	hvutil_transport_destroy(hvt);

[PATCH 4/4] hv: vss: use wrapper to propate state

[Olaf Hering]

The .state is used by several threads of execution.
Propagate the state to make changes visible. Also propagate context
change in vss_on_msg.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 drivers/hv/hv_snapshot.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 815405f..f3cb822 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -48,7 +48,7 @@
  */
 
 static struct {
-	int state;   /* hvutil_device_state */
+	enum hvutil_device_state state;
 	int recv_len; /* number of bytes received. */
 	struct vmbus_channel *recv_channel; /* chn we got the request */
 	u64 recv_req_id; /* request ID. */
@@ -64,6 +64,9 @@ static void vss_respond_to_host(int error);
  */
 static int dm_reg_value;
 
+#define vss_get_state() hvutil_device_get_state(&vss_transaction.state)
+#define vss_set_state(s) hvutil_device_set_state(&vss_transaction.state, s)
+
 static const char vss_devname[] = "vmbus/hv_vss";
 static __u8 *recv_buffer;
 static struct hvutil_transport *hvt;
@@ -87,8 +90,8 @@ static void vss_timeout_func(struct work_struct *dummy)
 	vss_respond_to_host(HV_E_FAIL);
 
 	/* Transaction is finished, reset the state. */
-	if (vss_transaction.state > HVUTIL_READY)
-		vss_transaction.state = HVUTIL_READY;
+	if (vss_get_state() > HVUTIL_READY)
+		vss_set_state(HVUTIL_READY);
 
 	hv_poll_channel(vss_transaction.vss_context,
 			hv_vss_onchannelcallback);
@@ -112,7 +115,7 @@ static int vss_handle_handshake(struct hv_vss_msg *vss_msg)
 	default:
 		return -EINVAL;
 	}
-	vss_transaction.state = HVUTIL_READY;
+	vss_set_state(HVUTIL_READY);
 	pr_debug("VSS: userspace daemon ver. %d registered\n", dm_reg_value);
 	return 0;
 }
@@ -130,15 +133,15 @@ static int vss_on_msg(void *msg, int len)
 		 * Don't process registration messages if we're in the middle
 		 * of a transaction processing.
 		 */
-		if (vss_transaction.state > HVUTIL_READY)
+		if (vss_get_state() > HVUTIL_READY)
 			return -EINVAL;
 		return vss_handle_handshake(vss_msg);
-	} else if (vss_transaction.state == HVUTIL_USERSPACE_REQ) {
-		vss_transaction.state = HVUTIL_USERSPACE_RECV;
+	} else if (vss_get_state() == HVUTIL_USERSPACE_REQ) {
+		vss_set_state(HVUTIL_USERSPACE_RECV);
 		if (cancel_delayed_work_sync(&vss_timeout_work)) {
 			vss_respond_to_host(vss_msg->error);
 			/* Transaction is finished, reset the state. */
-			vss_transaction.state = HVUTIL_READY;
+			vss_set_state(HVUTIL_READY);
 			hv_poll_channel(vss_transaction.vss_context,
 					hv_vss_onchannelcallback);
 		}
@@ -158,7 +161,7 @@ static void vss_send_op(struct work_struct *dummy)
 	struct hv_vss_msg *vss_msg;
 
 	/* The transaction state is wrong. */
-	if (vss_transaction.state != HVUTIL_HOSTMSG_RECEIVED)
+	if (vss_get_state() != HVUTIL_HOSTMSG_RECEIVED)
 		return;
 
 	vss_msg = kzalloc(sizeof(*vss_msg), GFP_KERNEL);
@@ -167,13 +170,13 @@ static void vss_send_op(struct work_struct *dummy)
 
 	vss_msg->vss_hdr.operation = op;
 
-	vss_transaction.state = HVUTIL_USERSPACE_REQ;
+	vss_set_state(HVUTIL_USERSPACE_REQ);
 	rc = hvutil_transport_send(hvt, vss_msg, sizeof(*vss_msg));
 	if (rc) {
 		pr_warn("VSS: failed to communicate to the daemon: %d\n", rc);
 		if (cancel_delayed_work_sync(&vss_timeout_work)) {
 			vss_respond_to_host(HV_E_FAIL);
-			vss_transaction.state = HVUTIL_READY;
+			vss_set_state(HVUTIL_READY);
 		}
 	}
 
@@ -238,7 +241,7 @@ void hv_vss_onchannelcallback(void *context)
 	struct icmsg_hdr *icmsghdrp;
 	struct icmsg_negotiate *negop = NULL;
 
-	if (vss_transaction.state > HVUTIL_READY) {
+	if (vss_get_state() > HVUTIL_READY) {
 		/*
 		 * We will defer processing this callback once
 		 * the current transaction is complete.
@@ -288,12 +291,12 @@ void hv_vss_onchannelcallback(void *context)
 				 */
 			case VSS_OP_FREEZE:
 			case VSS_OP_THAW:
-				if (vss_transaction.state < HVUTIL_READY) {
+				if (vss_get_state() < HVUTIL_READY) {
 					/* Userspace is not registered yet */
 					vss_respond_to_host(HV_E_FAIL);
 					return;
 				}
-				vss_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
+				vss_set_state(HVUTIL_HOSTMSG_RECEIVED);
 				schedule_work(&vss_send_op_work);
 				schedule_delayed_work(&vss_timeout_work,
 						      VSS_USERSPACE_TIMEOUT);
@@ -332,7 +335,7 @@ static void vss_on_reset(void)
 {
 	if (cancel_delayed_work_sync(&vss_timeout_work))
 		vss_respond_to_host(HV_E_FAIL);
-	vss_transaction.state = HVUTIL_DEVICE_INIT;
+	vss_set_state(HVUTIL_DEVICE_INIT);
 }
 
 int
@@ -346,7 +349,7 @@ hv_vss_init(struct hv_util_service *srv)
 	 * Defer processing channel callbacks until the daemon
 	 * has registered.
 	 */
-	vss_transaction.state = HVUTIL_DEVICE_INIT;
+	vss_set_state(HVUTIL_DEVICE_INIT);
 
 	hvt = hvutil_transport_init(vss_devname, CN_VSS_IDX, CN_VSS_VAL,
 				    vss_on_msg, vss_on_reset);
@@ -358,7 +361,7 @@ hv_vss_init(struct hv_util_service *srv)
 
 void hv_vss_deinit(void)
 {
-	vss_transaction.state = HVUTIL_DEVICE_DYING;
+	vss_set_state(HVUTIL_DEVICE_DYING);
 	cancel_delayed_work_sync(&vss_timeout_work);
 	cancel_work_sync(&vss_send_op_work);
 	hvutil_transport_destroy(hvt);