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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel