xenbus: Make xenbus_switch_state transactional

According to the comments, this was how it's been done years ago, but apparently took an xbt pointer from elsewhere back then. The code was removed because of consistency issues: cancellation wont't roll back the saved xbdev->state. Still, unsolicited writes to the state field remain an issue, especially if device shutdown takes thread synchronization, and subtle races cause accidental recreation of the device node. Fixed by reintroducing the transaction. An internal one is sufficient, so the xbdev->state value remains consistent. Also fixes the original hack to prevent infinite recursion. Instead of bailing out on the first attempt to switch to Closing, checks call depth now. Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

xenbus: Make xenbus_switch_state transactional
According to the comments, this was how it's been done years ago, but apparently took an xbt pointer from elsewhere back then. The code was removed because of consistency issues: cancellation wont't roll back the saved xbdev->state. Still, unsolicited writes to the state field remain an issue, especially if device shutdown takes thread synchronization, and subtle races cause accidental recreation of the device node. Fixed by reintroducing the transaction. An internal one is sufficient, so the xbdev->state value remains consistent. Also fixes the original hack to prevent infinite recursion. Instead of bailing out on the first attempt to switch to Closing, checks call depth now. Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Daniel Stodden · Jens Axboe
1 parent 2def141e71
Showing 1 changed file with 66 additions and 24 deletions Side-by-side Diff
drivers/xen/xenbus/xenbus_client.c
@@ -133,17 +133,12 @@
 }
 EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
  
+static void xenbus_switch_fatal(struct xenbus_device *, int, int,
+				const char *, ...);
  
-/**
- * xenbus_switch_state
- * @dev: xenbus device
- * @state: new state
- *
- * Advertise in the store a change of the given driver to the given new_state.
- * Return 0 on success, or -errno on error.  On error, the device will switch
- * to XenbusStateClosing, and the error will be saved in the store.
- */
-int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
+static int
+__xenbus_switch_state(struct xenbus_device *dev,
+		      enum xenbus_state state, int depth)
 {
 	/* We check whether the state is currently set to the given value, and
 	   if not, then the state is set.  We don't want to unconditionally
  
  
  
  
  
  
  
  
@@ -152,35 +147,65 @@
 	   to it, as the device will be tearing down, and we don't want to
 	   resurrect that directory.
  
-	   Note that, because of this cached value of our state, this function
-	   will not work inside a Xenstore transaction (something it was
-	   trying to in the past) because dev->state would not get reset if
-	   the transaction was aborted.
-
+	   Note that, because of this cached value of our state, this
+	   function will not take a caller's Xenstore transaction
+	   (something it was trying to in the past) because dev->state
+	   would not get reset if the transaction was aborted.
 	 */
  
+	struct xenbus_transaction xbt;
 	int current_state;
-	int err;
+	int err, abort;
  
 	if (state == dev->state)
 		return 0;
  
-	err = xenbus_scanf(XBT_NIL, dev->nodename, "state", "%d",
-			   &current_state);
-	if (err != 1)
+again:
+	abort = 1;
+
+	err = xenbus_transaction_start(&xbt);
+	if (err) {
+		xenbus_switch_fatal(dev, depth, err, "starting transaction");
 		return 0;
+	}
  
-	err = xenbus_printf(XBT_NIL, dev->nodename, "state", "%d", state);
+	err = xenbus_scanf(xbt, dev->nodename, "state", "%d", &current_state);
+	if (err != 1)
+		goto abort;
+
+	err = xenbus_printf(xbt, dev->nodename, "state", "%d", state);
 	if (err) {
-		if (state != XenbusStateClosing) /* Avoid looping */
-			xenbus_dev_fatal(dev, err, "writing new state");
-		return err;
+		xenbus_switch_fatal(dev, depth, err, "writing new state");
+		goto abort;
 	}
  
-	dev->state = state;
+	abort = 0;
+abort:
+	err = xenbus_transaction_end(xbt, abort);
+	if (err) {
+		if (err == -EAGAIN && !abort)
+			goto again;
+		xenbus_switch_fatal(dev, depth, err, "ending transaction");
+	} else
+		dev->state = state;
  
 	return 0;
 }
+
+/**
+ * xenbus_switch_state
+ * @dev: xenbus device
+ * @state: new state
+ *
+ * Advertise in the store a change of the given driver to the given new_state.
+ * Return 0 on success, or -errno on error.  On error, the device will switch
+ * to XenbusStateClosing, and the error will be saved in the store.
+ */
+int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
+{
+	return __xenbus_switch_state(dev, state, 0);
+}
+
 EXPORT_SYMBOL_GPL(xenbus_switch_state);
  
 int xenbus_frontend_closed(struct xenbus_device *dev)
@@ -282,6 +307,23 @@
 	xenbus_switch_state(dev, XenbusStateClosing);
 }
 EXPORT_SYMBOL_GPL(xenbus_dev_fatal);
+
+/**
+ * Equivalent to xenbus_dev_fatal(dev, err, fmt, args), but helps
+ * avoiding recursion within xenbus_switch_state.
+ */
+static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
+				const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	xenbus_va_dev_error(dev, err, fmt, ap);
+	va_end(ap);
+
+	if (!depth)
+		__xenbus_switch_state(dev, XenbusStateClosing, 1);
+}
  
 /**
  * xenbus_grant_ring
...	...	@@ -133,17 +133,12 @@
133	133	}
134	134	EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
135	135
	136	+static void xenbus_switch_fatal(struct xenbus_device *, int, int,
	137	+ const char *, ...);
136	138
137		-/**
138		- * xenbus_switch_state
139		- * @dev: xenbus device
140		- * @state: new state
141		- *
142		- * Advertise in the store a change of the given driver to the given new_state.
143		- * Return 0 on success, or -errno on error. On error, the device will switch
144		- * to XenbusStateClosing, and the error will be saved in the store.
145		- */
146		-int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
	139	+static int
	140	+__xenbus_switch_state(struct xenbus_device *dev,
	141	+ enum xenbus_state state, int depth)
147	142	{
148	143	/* We check whether the state is currently set to the given value, and
149	144	if not, then the state is set. We don't want to unconditionally
150	145
151	146
152	147
153	148
154	149
155	150
156	151
157	152
...	...	@@ -152,35 +147,65 @@
152	147	to it, as the device will be tearing down, and we don't want to
153	148	resurrect that directory.
154	149
155		- Note that, because of this cached value of our state, this function
156		- will not work inside a Xenstore transaction (something it was
157		- trying to in the past) because dev->state would not get reset if
158		- the transaction was aborted.
159		-
	150	+ Note that, because of this cached value of our state, this
	151	+ function will not take a caller's Xenstore transaction
	152	+ (something it was trying to in the past) because dev->state
	153	+ would not get reset if the transaction was aborted.
160	154	*/
161	155
	156	+ struct xenbus_transaction xbt;
162	157	int current_state;
163		- int err;
	158	+ int err, abort;
164	159
165	160	if (state == dev->state)
166	161	return 0;
167	162
168		- err = xenbus_scanf(XBT_NIL, dev->nodename, "state", "%d",
169		- &current_state);
170		- if (err != 1)
	163	+again:
	164	+ abort = 1;
	165	+
	166	+ err = xenbus_transaction_start(&xbt);
	167	+ if (err) {
	168	+ xenbus_switch_fatal(dev, depth, err, "starting transaction");
171	169	return 0;
	170	+ }
172	171
173		- err = xenbus_printf(XBT_NIL, dev->nodename, "state", "%d", state);
	172	+ err = xenbus_scanf(xbt, dev->nodename, "state", "%d", &current_state);
	173	+ if (err != 1)
	174	+ goto abort;
	175	+
	176	+ err = xenbus_printf(xbt, dev->nodename, "state", "%d", state);
174	177	if (err) {
175		- if (state != XenbusStateClosing) /* Avoid looping */
176		- xenbus_dev_fatal(dev, err, "writing new state");
177		- return err;
	178	+ xenbus_switch_fatal(dev, depth, err, "writing new state");
	179	+ goto abort;
178	180	}
179	181
180		- dev->state = state;
	182	+ abort = 0;
	183	+abort:
	184	+ err = xenbus_transaction_end(xbt, abort);
	185	+ if (err) {
	186	+ if (err == -EAGAIN && !abort)
	187	+ goto again;
	188	+ xenbus_switch_fatal(dev, depth, err, "ending transaction");
	189	+ } else
	190	+ dev->state = state;
181	191
182	192	return 0;
183	193	}
	194	+
	195	+/**
	196	+ * xenbus_switch_state
	197	+ * @dev: xenbus device
	198	+ * @state: new state
	199	+ *
	200	+ * Advertise in the store a change of the given driver to the given new_state.
	201	+ * Return 0 on success, or -errno on error. On error, the device will switch
	202	+ * to XenbusStateClosing, and the error will be saved in the store.
	203	+ */
	204	+int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
	205	+{
	206	+ return __xenbus_switch_state(dev, state, 0);
	207	+}
	208	+
184	209	EXPORT_SYMBOL_GPL(xenbus_switch_state);
185	210
186	211	int xenbus_frontend_closed(struct xenbus_device *dev)
...	...	@@ -282,6 +307,23 @@
282	307	xenbus_switch_state(dev, XenbusStateClosing);
283	308	}
284	309	EXPORT_SYMBOL_GPL(xenbus_dev_fatal);
	310	+
	311	+/**
	312	+ * Equivalent to xenbus_dev_fatal(dev, err, fmt, args), but helps
	313	+ * avoiding recursion within xenbus_switch_state.
	314	+ */
	315	+static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
	316	+ const char *fmt, ...)
	317	+{
	318	+ va_list ap;
	319	+
	320	+ va_start(ap, fmt);
	321	+ xenbus_va_dev_error(dev, err, fmt, ap);
	322	+ va_end(ap);
	323	+
	324	+ if (!depth)
	325	+ __xenbus_switch_state(dev, XenbusStateClosing, 1);
	326	+}
285	327
286	328	/**
287	329	* xenbus_grant_ring