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

[Jonathan Davies <jonathan.davies@xxxxxxxxxx>]

Dear xen-devel,

There's a race condition in xenfs (xenstore driver) that causes
userspace utility xenstore-watch to crash.

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.

Here's what is happening:

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?

A cursory glance at drivers/xen/xenbus/xenbus_dev.c suggests that it
suffers from the same problem. Although I haven't haven't tested this,
I'd expect that it requires a very similar solution.

Jonathan


Take the (non-reentrant) reply_mutex before calling
register_xenbus_watch to prevent the watch_fired callback from writing
anything until the "OK" has been sent.

Signed-off-by: Jonathan Davies <jonathan.davies@xxxxxxxxxx>

diff -r 5ab1b4af1faf drivers/xen/xenfs/xenbus.c
--- 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);
+
        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);
        return rc;
 }


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