[PATCH] xenstore: Fix deadlock in xs_read_watch

[PATCH] xenstore: Fix deadlock in xs_read_watch

[Daniel De Graaf]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

When read_message returns -1 while a read is pending, an attempt is made
to lock h->request_mutex which is already locked by the reader. Change
the read thread to only exit on thread cancellation, where
read_thr_exists will return 0.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

-- 

Daniel De Graaf
National Security Agency

[-- Attachment #2: deadlock-fix.patch --]
[-- Type: text/plain, Size: 1091 bytes --]

diff -r 6bebaf40e925 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Tue Jul 20 13:42:17 2010 +0100
+++ b/tools/xenstore/xs.c	Thu Aug 26 17:08:55 2010 -0400
@@ -271,6 +271,7 @@
 {
 #ifdef USE_PTHREAD
 	if (h->read_thr_exists) {
+		h->read_thr_exists = 0;
 		pthread_cancel(h->read_thr);
 		pthread_join(h->read_thr, NULL);
 	}
@@ -667,7 +668,7 @@
 	mutex_lock(&h->watch_mutex);
 
 	/* Wait on the condition variable for a watch to fire. */
-	while (list_empty(&h->watch_list) && read_thread_exists(h))
+	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
 		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
 	if (!read_thread_exists(h)) {
 		mutex_unlock(&h->watch_mutex);
@@ -966,13 +967,8 @@
 {
 	struct xs_handle *h = arg;
 
-	while (read_message(h) != -1)
-		continue;
-
-	/* Kick anyone waiting for a reply */
-	pthread_mutex_lock(&h->request_mutex);
-	h->read_thr_exists = 0;
-	pthread_mutex_unlock(&h->request_mutex);
+	while (h->read_thr_exists)
+		read_message(h);
 
 	pthread_mutex_lock(&h->reply_mutex);
 	pthread_cond_signal(&h->reply_condvar);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] xenstore: Fix deadlock in xs_read_watch

[Stefano Stabellini]

On Thu, 26 Aug 2010, Daniel De Graaf wrote:
> When read_message returns -1 while a read is pending, an attempt is made
> to lock h->request_mutex which is already locked by the reader. Change
> the read thread to only exit on thread cancellation, where
> read_thr_exists will return 0.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> diff -r 6bebaf40e925 tools/xenstore/xs.c
> --- a/tools/xenstore/xs.c	Tue Jul 20 13:42:17 2010 +0100
> +++ b/tools/xenstore/xs.c	Thu Aug 26 17:08:55 2010 -0400
> @@ -271,6 +271,7 @@
>  {
>  #ifdef USE_PTHREAD
>  	if (h->read_thr_exists) {
> +		h->read_thr_exists = 0;
>  		pthread_cancel(h->read_thr);
>  		pthread_join(h->read_thr, NULL);
>  	}
> @@ -667,7 +668,7 @@
>  	mutex_lock(&h->watch_mutex);
>  
>  	/* Wait on the condition variable for a watch to fire. */
> -	while (list_empty(&h->watch_list) && read_thread_exists(h))
> +	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
>  		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
>  	if (!read_thread_exists(h)) {
>  		mutex_unlock(&h->watch_mutex);

read_from_thread is not declared in this function

Re: [PATCH, v2] xenstore: Fix deadlock in xs_read_watch

[Daniel De Graaf]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On 08/30/2010 09:16 AM, Stefano Stabellini wrote:
> On Thu, 26 Aug 2010, Daniel De Graaf wrote:
>> When read_message returns -1 while a read is pending, an attempt is made
>> to lock h->request_mutex which is already locked by the reader. Change
>> the read thread to only exit on thread cancellation, where
>> read_thr_exists will return 0.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>
>> diff -r 6bebaf40e925 tools/xenstore/xs.c
>> --- a/tools/xenstore/xs.c	Tue Jul 20 13:42:17 2010 +0100
>> +++ b/tools/xenstore/xs.c	Thu Aug 26 17:08:55 2010 -0400
>> @@ -271,6 +271,7 @@
>>  {
>>  #ifdef USE_PTHREAD
>>  	if (h->read_thr_exists) {
>> +		h->read_thr_exists = 0;
>>  		pthread_cancel(h->read_thr);
>>  		pthread_join(h->read_thr, NULL);
>>  	}
>> @@ -667,7 +668,7 @@
>>  	mutex_lock(&h->watch_mutex);
>>  
>>  	/* Wait on the condition variable for a watch to fire. */
>> -	while (list_empty(&h->watch_list) && read_thread_exists(h))
>> +	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
>>  		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
>>  	if (!read_thread_exists(h)) {
>>  		mutex_unlock(&h->watch_mutex);
> 
> read_from_thread is not declared in this function
> 

Sorry about that, looks like I picked the wrong patch to send. Corrected
patch attached.

[-- Attachment #2: deadlock-fix.patch --]
[-- Type: text/plain, Size: 1123 bytes --]

diff -r 3c4c3d48a835 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c	Thu Aug 26 11:16:56 2010 +0100
+++ b/tools/xenstore/xs.c	Mon Aug 30 09:31:27 2010 -0400
@@ -271,6 +271,7 @@
 {
 #ifdef USE_PTHREAD
 	if (h->read_thr_exists) {
+		h->read_thr_exists = 0;
 		pthread_cancel(h->read_thr);
 		pthread_join(h->read_thr, NULL);
 	}
@@ -666,9 +667,15 @@
 
 	mutex_lock(&h->watch_mutex);
 
+#ifdef USE_PTHREAD
 	/* Wait on the condition variable for a watch to fire. */
 	while (list_empty(&h->watch_list) && read_thread_exists(h))
 		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
+#else
+	while (list_empty(&h->watch_list))
+		read_message(h);
+#endif
+	
 	if (!read_thread_exists(h)) {
 		mutex_unlock(&h->watch_mutex);
 		errno = EINVAL;
@@ -966,13 +973,8 @@
 {
 	struct xs_handle *h = arg;
 
-	while (read_message(h) != -1)
-		continue;
-
-	/* Kick anyone waiting for a reply */
-	pthread_mutex_lock(&h->request_mutex);
-	h->read_thr_exists = 0;
-	pthread_mutex_unlock(&h->request_mutex);
+	while (h->read_thr_exists)
+		read_message(h);
 
 	pthread_mutex_lock(&h->reply_mutex);
 	pthread_cond_signal(&h->reply_condvar);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH, v2] xenstore: Fix deadlock in xs_read_watch

[Stefano Stabellini]

On Mon, 30 Aug 2010, Daniel De Graaf wrote:
> On 08/30/2010 09:16 AM, Stefano Stabellini wrote:
> > On Thu, 26 Aug 2010, Daniel De Graaf wrote:
> >> When read_message returns -1 while a read is pending, an attempt is made
> >> to lock h->request_mutex which is already locked by the reader. Change
> >> the read thread to only exit on thread cancellation, where
> >> read_thr_exists will return 0.
> >>
> >> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> >>
> >> diff -r 6bebaf40e925 tools/xenstore/xs.c
> >> --- a/tools/xenstore/xs.c	Tue Jul 20 13:42:17 2010 +0100
> >> +++ b/tools/xenstore/xs.c	Thu Aug 26 17:08:55 2010 -0400
> >> @@ -271,6 +271,7 @@
> >>  {
> >>  #ifdef USE_PTHREAD
> >>  	if (h->read_thr_exists) {
> >> +		h->read_thr_exists = 0;
> >>  		pthread_cancel(h->read_thr);
> >>  		pthread_join(h->read_thr, NULL);
> >>  	}
> >> @@ -667,7 +668,7 @@
> >>  	mutex_lock(&h->watch_mutex);
> >>  
> >>  	/* Wait on the condition variable for a watch to fire. */
> >> -	while (list_empty(&h->watch_list) && read_thread_exists(h))
> >> +	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
> >>  		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
> >>  	if (!read_thread_exists(h)) {
> >>  		mutex_unlock(&h->watch_mutex);
> > 
> > read_from_thread is not declared in this function
> > 
> 
> Sorry about that, looks like I picked the wrong patch to send. Corrected
> patch attached.
> 

According to the description you gave, this patch fixes a problem
occurring when read_message returns -1 while a read is pending in
read_thread that is relevant only when USE_PTHREAD.
Why are you changing a code path that is only compiled when
!USE_PTHREAD?
Also shouldn't you be checking the return value of read_message anyway?
Considering that h->read_thr_exists is set to zero only on
xs_daemon_close, and in that case pthread_cancel is called right after,
I doubt that any of the operation run in read_thread after the loop will
be executed.

Re: [PATCH, v2] xenstore: Fix deadlock in xs_read_watch

[Daniel De Graaf]

On 08/30/2010 09:52 AM, Stefano Stabellini wrote:
> On Mon, 30 Aug 2010, Daniel De Graaf wrote:
>> On 08/30/2010 09:16 AM, Stefano Stabellini wrote:
>>> On Thu, 26 Aug 2010, Daniel De Graaf wrote:
>>>> When read_message returns -1 while a read is pending, an attempt is made
>>>> to lock h->request_mutex which is already locked by the reader. Change
>>>> the read thread to only exit on thread cancellation, where
>>>> read_thr_exists will return 0.
>>>>
>>>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>>
>>>> diff -r 6bebaf40e925 tools/xenstore/xs.c
>>>> --- a/tools/xenstore/xs.c	Tue Jul 20 13:42:17 2010 +0100
>>>> +++ b/tools/xenstore/xs.c	Thu Aug 26 17:08:55 2010 -0400
>>>> @@ -271,6 +271,7 @@
>>>>  {
>>>>  #ifdef USE_PTHREAD
>>>>  	if (h->read_thr_exists) {
>>>> +		h->read_thr_exists = 0;
>>>>  		pthread_cancel(h->read_thr);
>>>>  		pthread_join(h->read_thr, NULL);
>>>>  	}
>>>> @@ -667,7 +668,7 @@
>>>>  	mutex_lock(&h->watch_mutex);
>>>>  
>>>>  	/* Wait on the condition variable for a watch to fire. */
>>>> -	while (list_empty(&h->watch_list) && read_thread_exists(h))
>>>> +	while (list_empty(&h->watch_list) && (!read_from_thread || read_thread_exists(h)))
>>>>  		condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
>>>>  	if (!read_thread_exists(h)) {
>>>>  		mutex_unlock(&h->watch_mutex);
>>>
>>> read_from_thread is not declared in this function
>>>
>>
>> Sorry about that, looks like I picked the wrong patch to send. Corrected
>> patch attached.
>>
> 
> According to the description you gave, this patch fixes a problem
> occurring when read_message returns -1 while a read is pending in
> read_thread that is relevant only when USE_PTHREAD.
> Why are you changing a code path that is only compiled when
> !USE_PTHREAD?
> Also shouldn't you be checking the return value of read_message anyway?
> Considering that h->read_thr_exists is set to zero only on
> xs_daemon_close, and in that case pthread_cancel is called right after,
> I doubt that any of the operation run in read_thread after the loop will
> be executed.
> 

The value of read_message is not consistently checked in existing code.
I have submitted a separate patch that addresses this and also fixes the
deadlock, so neither of the patches in this thread are required.

-- 

Daniel De Graaf
National Security Agency