Re: [Xen-devel] xenfs: race condition on xenstore watch

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.05.13 at 19:07, Jonathan Davies <jonathan.davies@xxxxxxxxxx> wrote:
> On 31/05/13 14:45, Jan Beulich wrote:
>>>>> On 28.05.13 at 14:55, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 
>>>>> wrote:
>>> On Wed, May 15, 2013 at 05:51:06PM +0100, Jonathan Davies wrote:
>>>> Normally, the userspace process gets an "OK" from xenfs and then the
>>>> watch fires immediately after. Occasionally, this happens the other way
>>>> around: the watch fires before the driver sends "OK", which confuses
>>>> the xenstore-watch client. It seems to me that the client is within its
>>>> rights to expect the "OK" first.
>>
>> Now that I look at this another time, I wonder how this behavior
>> can be guaranteed even with your patch: xenbus_write_watch()
>> sends the reply by calling queue_reply() followed by wake_up(),
>> so the reply getting delivered and the watch firing appear to be
>> inherently asynchronous anyway, and your patch just reduces
>> the race window. Am I overlooking something here?
> 
> There's no call to wake_up() in xenbus_write_watch(), as far as I can see...

I'm not sure where you're looking, but both xenfs/xenbus.c and
the more recent xenbus/xenbus_dev_frontend.c have this

                mutex_lock(&u->reply_mutex);
                rc = queue_reply(&u->read_buffers, &reply, sizeof(reply));
                wake_up(&u->read_waitq);
                mutex_unlock(&u->reply_mutex);

near the end of xenbus_write_watch().

> The key thing is to guarantee that xenbus_write_watch()'s call to 
> queue_reply() (for the "OK" message) executes before the watch_fired() 
> callback -- registered by the call to register_xenbus_watch() -- could be 
> called-back. Given that watch_fired() also takes the reply_mutex before its 
> own calls to queue_reply() and wake_up(), this behaviour can be guaranteed 
> by taking the reply_mutex before registering the callback.

Okay, I agree after taking another closer look - the very different
variable naming obfuscate this a little. And inn fact the mutex
appears to get acquired too early in watch_fired - it could be
confined to just the list_splice_tail() (and maybe the wake_up(), if
the really requires external synchronization) invocation.

Jan


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