Daniel De Graaf
2010-Aug-26 21:16 UTC
[Xen-devel] [PATCH] xenstore: Fix deadlock in xs_read_watch
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Aug-30 13:16 UTC
Re: [Xen-devel] [PATCH] xenstore: Fix deadlock in xs_read_watch
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2010-Aug-30 13:36 UTC
Re: [Xen-devel] [PATCH, v2] xenstore: Fix deadlock in xs_read_watch
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Aug-30 13:52 UTC
Re: [Xen-devel] [PATCH, v2] xenstore: Fix deadlock in xs_read_watch
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel De Graaf
2010-Aug-30 15:05 UTC
Re: [Xen-devel] [PATCH, v2] xenstore: Fix deadlock in xs_read_watch
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel