Re: xenfs: race condition on xenstore watch

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, May 15, 2013 at 05:51:06PM +0100, Jonathan Davies wrote:
> 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?

It can't apply cleanly as the file moved :-(

> 
> 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.

Should that perhaps be done inside the msg_type == XS_WATCH code (with a
bool that would determine whether an reply_mutex has been taken?)

As in, is there no need to take this mutex if msg_type != XS_WATCH?

> 
> Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>
> 
> 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);
> +

Have you tested this with the kernel compiled with DEBUG_MUTEX and DEBUG_PROVE_LOCKING
to make sure there are no mutex/spinlock issues?

>  	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@lists.xen.org
> http://lists.xen.org/xen-devel
>