From: Daniel De Graaf Subject: Re: [PATCH, v2] xenstore: Fix deadlock in xs_read_watch Date: Mon, 30 Aug 2010 11:05:30 -0400 Message-ID: <4C7BC8BA.4090300@tycho.nsa.gov> References: <4C76D9C3.5020600@tycho.nsa.gov> <4C7BB3E6.1080406@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Stefano Stabellini Cc: xen-devel List-Id: xen-devel@lists.xenproject.org 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 >>>> >>>> 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