From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Creekmore Subject: [PATCH] libxenstore: Use poll() with a non-blocking read() Date: Thu, 13 Aug 2015 16:44:47 -0500 Message-ID: <1439502287-5520-1-git-send-email-jonathan.creekmore@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZQ0KY-0005Nu-8g for xen-devel@lists.xenproject.org; Thu, 13 Aug 2015 21:46:34 +0000 Received: by qkbm65 with SMTP id m65so20154105qkb.2 for ; Thu, 13 Aug 2015 14:46:31 -0700 (PDT) List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xenproject.org Cc: wei.liu@citrix.com, Jonathan Creekmore , ian.jackson@eu.citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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 --- 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 #include #include +#include #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