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

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

[Jonathan Creekmore]

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

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

[Ian Campbell]

On Thu, 2015-08-13 at 16:44 -0500, Jonathan Creekmore wrote:
> 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 sounds like you are describing a kernel bug. Shouldn't this be
fixed in the kernel?

In fact it even sounds a bit familiar, I wonder if it is fixed in some
version of Linux >> 3.14? (CCing a few relevant maintainers)

>  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

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

[Jonathan Creekmore]

> On Aug 16, 2015, at 1:59 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> 
> On Thu, 2015-08-13 at 16:44 -0500, Jonathan Creekmore wrote:
>> 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 sounds like you are describing a kernel bug. Shouldn't this be
> fixed in the kernel?
> 
> In fact it even sounds a bit familiar, I wonder if it is fixed in some
> version of Linux >> 3.14? (CCing a few relevant maintainers)
> 

So, the latest I saw on the LKML, the problem still existed as of 3.17 and, looking forward through 4.2, the problem still exists there as well. I saw a few posts back in March 2015 talking about the deadlock, but it appeared to have gotten no traction. It isn’t a deadlock in the kernel, but rather a deadlock between the two blocking reads or a blocking read or write. The slight rewrite I applied in my patch effectively works around the problem and prevents the library from flip-flopping the nonblocking flag on the file descriptor between two threads. 


>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

On 08/16/2015 08:46 PM, Jonathan Creekmore wrote:
>> On Aug 16, 2015, at 1:59 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
>>
>> On Thu, 2015-08-13 at 16:44 -0500, Jonathan Creekmore wrote:
>>> 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 sounds like you are describing a kernel bug. Shouldn't this be
>> fixed in the kernel?

It is a bug in the sense that addition of FMODE_ATOMIC_POS changed 
user-visible behavior. And a fix was suggested in 
http://permalink.gmane.org/gmane.linux.file-systems/93969 .

Copying Vitalii who sent an early version of that patch.

-boris

>>
>> In fact it even sounds a bit familiar, I wonder if it is fixed in some
>> version of Linux >> 3.14? (CCing a few relevant maintainers)
>>
> So, the latest I saw on the LKML, the problem still existed as of 3.17 and, looking forward through 4.2, the problem still exists there as well. I saw a few posts back in March 2015 talking about the deadlock, but it appeared to have gotten no traction. It isn’t a deadlock in the kernel, but rather a deadlock between the two blocking reads or a blocking read or write. The slight rewrite I applied in my patch effectively works around the problem and prevents the library from flip-flopping the nonblocking flag on the file descriptor between two threads.
>
>
>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[David Vrabel]

On 16/08/15 09:59, Ian Campbell wrote:
> On Thu, 2015-08-13 at 16:44 -0500, Jonathan Creekmore wrote:
>> 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.

I think you should make libxenstore open /dev/xen/xenbus instead.  Since
this is a character device it should work correctly.

It may be necessary to try /dev/xen/xenbus first and fallback to
/proc/xen/xenbus.

> This sounds like you are describing a kernel bug. Shouldn't this be
> fixed in the kernel?

/proc/xen/xenbus should never have existed (it's a character device
masquerading as a regular file), but I guess we're stuck with it now.

The correct kernel fix is to make /proc/xen/xenbus a character device
identical to /dev/xen/xenbus or a symlink to /dev/xen/xenbus.

David

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

[Jonathan Creekmore]

David Vrabel <david.vrabel@citrix.com> writes:

> On 16/08/15 09:59, Ian Campbell wrote:
>> On Thu, 2015-08-13 at 16:44 -0500, Jonathan Creekmore wrote:
>>> 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.
>
> I think you should make libxenstore open /dev/xen/xenbus instead.  Since
> this is a character device it should work correctly.
>

So, I did a quick test and verified that 1) /dev/xen/xenbus did exist on
my system and 2) that opening /dev/xen/xenbus instead of
/proc/xen/xenbus fixed the problem I was seeing as well. I did think
that it was a little weird that libxenstore was treating a proc file as
a character device. It makes more sense to me to open the actual
character device instead. 

> It may be necessary to try /dev/xen/xenbus first and fallback to
> /proc/xen/xenbus.
>

When did /dev/xen/xenbus get created? It is a fairly straightforward
change to just switch to /dev/xen/xenbus and is a bit larger refactor to
probe for its existence first before falling back to
/proc/xen/xenbus.

Either way, I could work up a patch to do this if people are ok with
that user-space change. 


That said, I still think that the original patch I submitted is
worthwhile to prevent a subtle, but probably benign, race condition with
changing the blocking flag on a file descriptor that is shared between
two threads. 

[PATCH v2] libxenstore: prefer using the character device

[Jonathan Creekmore]

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. Accessing the daemon from the true character device
also does not exhibit this problem.

On Linux, prefer using the character device file over the proc file if
the character device exists.

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

diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
index af4f75a..0c7744e 100644
--- a/tools/xenstore/xs_lib.c
+++ b/tools/xenstore/xs_lib.c
@@ -81,6 +81,8 @@ const char *xs_domain_dev(void)
 #if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
 	return "/dev/xen/xenbus";
 #elif defined(__linux__)
+	if (access("/dev/xen/xenbus", F_OK) == 0)
+		return "/dev/xen/xenbus";
 	return "/proc/xen/xenbus";
 #elif defined(__NetBSD__)
 	return "/kern/xen/xenbus";
-- 
2.1.4

Re: [PATCH v2] libxenstore: prefer using the character device

[Wei Liu]

On Thu, Aug 27, 2015 at 09:04:38AM -0500, Jonathan Creekmore wrote:
> 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. Accessing the daemon from the true character device
> also does not exhibit this problem.
> 
> On Linux, prefer using the character device file over the proc file if
> the character device exists.
> 
> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

I think this is a candidate for 4.6.

> ---
>  tools/xenstore/xs_lib.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
> index af4f75a..0c7744e 100644
> --- a/tools/xenstore/xs_lib.c
> +++ b/tools/xenstore/xs_lib.c
> @@ -81,6 +81,8 @@ const char *xs_domain_dev(void)
>  #if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
>  	return "/dev/xen/xenbus";
>  #elif defined(__linux__)
> +	if (access("/dev/xen/xenbus", F_OK) == 0)
> +		return "/dev/xen/xenbus";
>  	return "/proc/xen/xenbus";
>  #elif defined(__NetBSD__)
>  	return "/kern/xen/xenbus";
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2] libxenstore: prefer using the character device

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH v2] libxenstore: prefer using the character device"):
> On Thu, Aug 27, 2015 at 09:04:38AM -0500, Jonathan Creekmore wrote:
> > 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. Accessing the daemon from the true character device
> > also does not exhibit this problem.
> > 
> > On Linux, prefer using the character device file over the proc file if
> > the character device exists.

I confess I still see this as working around a kernel bug.  Only this
time we are switching from a buggy to non-buggy kernel interface.

Why don't we have the kernel provide only non-buggy interfaces ?

> > diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
> > index af4f75a..0c7744e 100644
> > --- a/tools/xenstore/xs_lib.c
> > +++ b/tools/xenstore/xs_lib.c
> > @@ -81,6 +81,8 @@ const char *xs_domain_dev(void)
> >  #if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
> >  	return "/dev/xen/xenbus";
> >  #elif defined(__linux__)
> > +	if (access("/dev/xen/xenbus", F_OK) == 0)
> > +		return "/dev/xen/xenbus";

Also, previously xs_domain_dev was a function which simply returned a
static value.  I feel vaguely uneasy at putting this kind of
autodetection logic here.

Ian.

Re: [PATCH v2] libxenstore: prefer using the character device

[Jonathan Creekmore]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] libxenstore: prefer 
> using the character device"): 
>> On Thu, Aug 27, 2015 at 09:04:38AM -0500, Jonathan Creekmore 
>> wrote: 
>> > 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. Accessing the daemon from the true 
>> > character device also does not exhibit this problem.   On 
>> > Linux, prefer using the character device file over the proc 
>> > file if the character device exists. 
> 
> I confess I still see this as working around a kernel bug.  Only 
> this time we are switching from a buggy to non-buggy kernel 
> interface. 
> 
> Why don't we have the kernel provide only non-buggy interfaces ?

I was just trying to implement what David suggested; CC'ing him on 
this as well.
 
> 
>> > diff --git a/tools/xenstore/xs_lib.c 
>> > b/tools/xenstore/xs_lib.c index af4f75a..0c7744e 100644 --- 
>> > a/tools/xenstore/xs_lib.c +++ b/tools/xenstore/xs_lib.c @@ 
>> > -81,6 +81,8 @@ const char *xs_domain_dev(void) 
>> >  #if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__) return 
>> >  "/dev/xen/xenbus"; #elif defined(__linux__) 
>> > +	if (access("/dev/xen/xenbus", F_OK) == 0) + 
>> > return "/dev/xen/xenbus"; 
> 
> Also, previously xs_domain_dev was a function which simply 
> returned a static value.  I feel vaguely uneasy at putting this 
> kind of autodetection logic here. 

Not entirely; the existing code queried an environment variable first
and, only if that was not set, did it return a static value. I added the
autodetection logic to fall back in the case where, for some reason,
/dev/xen/xenbus did not exist. Handling that kind of a fallback at a
higher layer would be a larger refactor to probe for the existence of
the character device first.

Re: [PATCH v2] libxenstore: prefer using the character device

[David Vrabel]

On 27/08/15 19:03, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] libxenstore: prefer using the character device"):
>> On Thu, Aug 27, 2015 at 09:04:38AM -0500, Jonathan Creekmore wrote:
>>> 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. Accessing the daemon from the true character device
>>> also does not exhibit this problem.
>>>
>>> On Linux, prefer using the character device file over the proc file if
>>> the character device exists.
> 
> I confess I still see this as working around a kernel bug.  Only this
> time we are switching from a buggy to non-buggy kernel interface.

/proc/xen/xenbus is deprecated.  The tools should use the non-deprecated
interface.

> Why don't we have the kernel provide only non-buggy interfaces ?

Fixing /proc/xen/xenbus is non-trival and since there's a fully working
non-deprecated interface (/dev/xen/xenbus), it's unlikely that anyone is
going to be inspired to fix it.

>>> diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
>>> index af4f75a..0c7744e 100644
>>> --- a/tools/xenstore/xs_lib.c
>>> +++ b/tools/xenstore/xs_lib.c
>>> @@ -81,6 +81,8 @@ const char *xs_domain_dev(void)
>>>  #if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
>>>  	return "/dev/xen/xenbus";
>>>  #elif defined(__linux__)
>>> +	if (access("/dev/xen/xenbus", F_OK) == 0)
>>> +		return "/dev/xen/xenbus";
> 
> Also, previously xs_domain_dev was a function which simply returned a
> static value.  I feel vaguely uneasy at putting this kind of
> autodetection logic here.

"Vaguely uneasy"?  Are we engineers or witchdoctors?

xs_domain_dev() already does a system call to query the environment so
it did not just "return a static value":

const char *xs_domain_dev(void)
{
        char *s = getenv("XENSTORED_PATH");
        if (s)
                return s;
        ...

David

Re: [PATCH v2] libxenstore: prefer using the character device

[Jonathan Creekmore]

Just wanted to follow-up and see if there was any more debate on 
this, since I hadn't seen any other commentary since last week.

David Vrabel <david.vrabel@citrix.com> writes:

> On 27/08/15 19:03, Ian Jackson wrote:
>> Wei Liu writes ("Re: [Xen-devel] [PATCH v2] libxenstore: prefer using the character device"):
>>> On Thu, Aug 27, 2015 at 09:04:38AM -0500, Jonathan Creekmore wrote:
>>>> 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. Accessing the daemon from the true character device
>>>> also does not exhibit this problem.
>>>>
>>>> On Linux, prefer using the character device file over the proc file if
>>>> the character device exists.
>> 
>> I confess I still see this as working around a kernel bug.  Only this
>> time we are switching from a buggy to non-buggy kernel interface.
>
> /proc/xen/xenbus is deprecated.  The tools should use the non-deprecated
> interface.
>
>> Why don't we have the kernel provide only non-buggy interfaces ?
>
> Fixing /proc/xen/xenbus is non-trival and since there's a fully working
> non-deprecated interface (/dev/xen/xenbus), it's unlikely that anyone is
> going to be inspired to fix it.
>
>>>> diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
>>>> index af4f75a..0c7744e 100644
>>>> --- a/tools/xenstore/xs_lib.c
>>>> +++ b/tools/xenstore/xs_lib.c
>>>> @@ -81,6 +81,8 @@ const char *xs_domain_dev(void)
>>>>  #if defined(__RUMPUSER_XEN__) || defined(__RUMPRUN__)
>>>>  	return "/dev/xen/xenbus";
>>>>  #elif defined(__linux__)
>>>> +	if (access("/dev/xen/xenbus", F_OK) == 0)
>>>> +		return "/dev/xen/xenbus";
>> 
>> Also, previously xs_domain_dev was a function which simply returned a
>> static value.  I feel vaguely uneasy at putting this kind of
>> autodetection logic here.
>
> "Vaguely uneasy"?  Are we engineers or witchdoctors?
>
> xs_domain_dev() already does a system call to query the environment so
> it did not just "return a static value":
>
> const char *xs_domain_dev(void)
> {
>         char *s = getenv("XENSTORED_PATH");
>         if (s)
>                 return s;
>         ...
>
> David

Re: [PATCH v2] libxenstore: prefer using the character device

[Wei Liu]

On Mon, Aug 31, 2015 at 01:59:24PM -0500, Jonathan Creekmore wrote:
> Just wanted to follow-up and see if there was any more debate on this, since
> I hadn't seen any other commentary since last week.
> 

After reading all replies to this thread I'm of the opinion that this
patch should be applied.

Wei.

Re: [PATCH v2] libxenstore: prefer using the character device

[Ian Jackson]

David Vrabel writes ("Re: [Xen-devel] [PATCH v2] libxenstore: prefer using the character device"):
> On 27/08/15 19:03, Ian Jackson wrote:
> > I confess I still see this as working around a kernel bug.  Only this
> > time we are switching from a buggy to non-buggy kernel interface.
> 
> /proc/xen/xenbus is deprecated.  The tools should use the non-deprecated
> interface.

Why don't we just change it, then ?  What kernels don't provide
/dev/xen/xenbus ?

> > Why don't we have the kernel provide only non-buggy interfaces ?
> 
> >>> +	if (access("/dev/xen/xenbus", F_OK) == 0)
> >>> +		return "/dev/xen/xenbus";
> > 
> > Also, previously xs_domain_dev was a function which simply returned a
> > static value.  I feel vaguely uneasy at putting this kind of
> > autodetection logic here.
> 
> "Vaguely uneasy"?  Are we engineers or witchdoctors?

It doesn't fit my mental model of what this function is for.  I think
it would be in better taste would be to arrange to attempt to call
open() on both strings.  But perhaps that is too much to do at this
stage of the release.

> xs_domain_dev() already does a system call to query the environment so
> it did not just "return a static value":

getenv is not a system call.


Anyway, if others don't have similar objections I am not nacking
this.  It may be applied with Wei's ack.

Ian.

Re: [PATCH v2] libxenstore: prefer using the character device

[Ian Campbell]

On Tue, 2015-09-01 at 12:28 +0100, Ian Jackson wrote:
> David Vrabel writes ("Re: [Xen-devel] [PATCH v2] libxenstore: prefer 
> using the character device"):
> > On 27/08/15 19:03, Ian Jackson wrote:
> > > I confess I still see this as working around a kernel bug.  Only this
> > > time we are switching from a buggy to non-buggy kernel interface.
> > 
> > /proc/xen/xenbus is deprecated.  The tools should use the non
> > -deprecated
> > interface.
> 
> Why don't we just change it, then ?  What kernels don't provide
> /dev/xen/xenbus ?

It was added by 2fb3683e7b164ee2b324039f7c9d90fe5b1a259b, which looks to be
circa 3.2ish (git describe --contains gave me a stupid answer, so not sure
when it actually hit mainline, 3.2-rc was in Makefile at that commit).

It's perhaps nearly time but I don't think we can quite discard e.g. 3.0
yet and I'm not sure of the status of the forwarded ported classic Xen
kernels.

BTW around the same time /dev/xen/foo replacements were added for most
/proc/xen functionality, e.g. privcmd and xenbus_backend (replacing two
xsd_* I think). Really we ought to be moving over.

> > > > > +	if (access("/dev/xen/xenbus", F_OK) == 0)
> > > > > +		return "/dev/xen/xenbus";
> > > 
> > > Also, previously xs_domain_dev was a function which simply returned a
> > > static value.  I feel vaguely uneasy at putting this kind of
> > > autodetection logic here.
> > 
> > "Vaguely uneasy"?  Are we engineers or witchdoctors?
> 
> It doesn't fit my mental model of what this function is for.  I think
> it would be in better taste would be to arrange to attempt to call
> open() on both strings.  But perhaps that is too much to do at this
> stage of the release.
> 
> > xs_domain_dev() already does a system call to query the environment so
> > it did not just "return a static value":
> 
> getenv is not a system call.
> 
> 
> Anyway, if others don't have similar objections I am not nacking
> this.  It may be applied with Wei's ack.

I've done so, with my Ack as well. thanks.

Ian.