[PATCH] libxenstore: Use poll() with a non-blocking read()

[Jonathan Creekmore <jonathan.creekmore@gmail.com>]

With the addition of FMODE_ATOMIC_POS in the Linux 3.14 kernel,
concurrent blocking file accesses to a single open file descriptor can
cause a deadlock trying to grab the file position lock. If a watch has
been set up, causing a read_thread to blocking read on the file
descriptor, then future writes that would cause the background read to
complete will block waiting on the file position lock before they can
execute. This race condition only occurs when libxenstore is accessing
the xenstore daemon through the /proc/xen/xenbus file and not through
the unix domain socket, which is the case when the xenstore daemon is
running as a stub domain or when oxenstored is passed --disable-socket.

Arguably, since the /proc/xen/xenbus file is declared nonseekable, then
the file position lock need not be grabbed, since the file cannot be
seeked. However, that is not how the kernel API works. On the other
hand, using the poll() API to implement the blocking for the read_all()
function prevents the file descriptor from being switched back and forth
between blocking and non-blocking modes between two threads.

Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>
---
 tools/xenstore/xs.c | 52 ++++++++++++++++------------------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index d1e01ba..9b75493 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -31,6 +31,7 @@
 #include <signal.h>
 #include <stdint.h>
 #include <errno.h>
+#include <poll.h>
 #include "xenstore.h"
 #include "list.h"
 #include "utils.h"
@@ -145,22 +146,6 @@ struct xs_handle {
 
 static int read_message(struct xs_handle *h, int nonblocking);
 
-static bool setnonblock(int fd, int nonblock) {
-	int flags = fcntl(fd, F_GETFL);
-	if (flags == -1)
-		return false;
-
-	if (nonblock)
-		flags |= O_NONBLOCK;
-	else
-		flags &= ~O_NONBLOCK;
-
-	if (fcntl(fd, F_SETFL, flags) == -1)
-		return false;
-
-	return true;
-}
-
 int xs_fileno(struct xs_handle *h)
 {
 	char c = 0;
@@ -216,7 +201,7 @@ error:
 static int get_dev(const char *connect_to)
 {
 	/* We cannot open read-only because requests are writes */
-	return open(connect_to, O_RDWR);
+	return open(connect_to, O_RDWR | O_NONBLOCK);
 }
 
 static struct xs_handle *get_handle(const char *connect_to)
@@ -365,42 +350,37 @@ static bool read_all(int fd, void *data, unsigned int len, int nonblocking)
 	/* With nonblocking, either reads either everything requested,
 	 * or nothing. */
 {
-	if (!len)
-		return true;
-
-	if (nonblocking && !setnonblock(fd, 1))
-		return false;
+	int done;
+	struct pollfd fds[] = {
+		{
+			.fd = fd,
+			.events = POLLIN
+		}
+	};
 
 	while (len) {
-		int done;
+		if (!nonblocking) {
+			if (poll(fds, 1, -1) < 1) {
+				return false;
+			}
+		}
 
 		done = read(fd, data, len);
 		if (done < 0) {
 			if (errno == EINTR)
 				continue;
-			goto out_false;
+			return false;
 		}
 		if (done == 0) {
 			/* It closed fd on us?  EBADF is appropriate. */
 			errno = EBADF;
-			goto out_false;
+			return false;
 		}
 		data += done;
 		len -= done;
-
-		if (nonblocking) {
-			nonblocking = 0;
-			if (!setnonblock(fd, 0))
-				goto out_false;
-		}
 	}
 
 	return true;
-
-out_false:
-	if (nonblocking)
-		setnonblock(fd, 0);
-	return false;
 }
 
 #ifdef XSTEST
-- 
2.1.4