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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.05.13 at 18:51, Jonathan Davies <jonathan.davies@xxxxxxxxxx> wrote:
> The userspace process xenstore-watch writes to /proc/xen/xenbus with
> msg_type==XS_WATCH. This is handled by xenbus_write_watch which calls
> register_xenbus_watch with watch_fired as a callback *before* acquiring
> the reply_mutex and sending the synthesised "OK" reply.
> 
> This gives a fast xenstore the opportunity to cause the watch_fired to
> run (and briefly grab the reply_mutex for itself) before the fake "OK"
> message is sent.
> 
> Below, I've included a putative patch for pre-3.3 xenfs that fixes this
> problem. (It looks like the patch would also apply cleanly to
> 3.3-onwards xenbus_dev_frontend.c, but I haven't tried.) Any comments
> about whether this is a reasonable approach?

Yes, this looks reasonable at a first glance, pending confirmation
by someone involved in the original xenbus/xenstore design that
the expectation to see the watch firing only after the OK response
is a valid one.

There's an implementation problem though:

> --- a/drivers/xen/xenfs/xenbus.c      Thu Dec 03 06:00:06 2009 +0000
> +++ b/drivers/xen/xenfs/xenbus.c      Wed May 15 17:24:47 2013 +0100
> @@ -359,6 +359,8 @@ static int xenbus_write_watch(unsigned m
>       }
>       token++;
> 
> +     mutex_lock(&u->reply_mutex);
> +

Right above the initial patch context there is another "goto out",
so ...

>       if (msg_type == XS_WATCH) {
>               watch = alloc_watch_adapter(path, token);
>               if (watch == NULL) {
> @@ -401,12 +403,11 @@ static int xenbus_write_watch(unsigned m
>                       "OK"
>               };
> 
> -             mutex_lock(&u->reply_mutex);
>               rc = queue_reply(&u->read_buffers, &reply, sizeof(reply));
> -             mutex_unlock(&u->reply_mutex);
>       }
> 
>   out:
> +     mutex_unlock(&u->reply_mutex);

... this is not valid.

>       return rc;
>   }
> 

Thus the unlock needs to become conditional here (either via
tracking the state in a local variable, or via adding a second label,
or via replacing the first goto with a straight return).

I'd also recommend to shrink the protected region to the minimum
possible (i.e. acquire the mutex right before the call to
register_xenbus_watch(), and at the end of the "else" body). Since
you then would end up with only a single error path needing to
drop the lock, dropping it in that error path rather than moving it
past the "out" label might be preferable.

Jan


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