From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: xenfs: race condition on xenstore watch Date: Tue, 28 May 2013 08:55:04 -0400 Message-ID: <20130528125504.GA28949@phenom.dumpdata.com> References: <5193BCFA.3030604@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <5193BCFA.3030604@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jonathan Davies Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > > 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 >