WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 2 of 3] linux-2.6.18: xen/pv-on-hvm kexec: preven

To: Jan Beulich <JBeulich@xxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 2 of 3] linux-2.6.18: xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when stale watch events arrive
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Thu, 6 Oct 2011 10:01:53 +0100
Cc: Olaf Hering <olaf@xxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Delivery-date: Thu, 06 Oct 2011 02:04:19 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4E8D88CE0200007800059A1A@xxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <patchbomb.1317823853@xxxxxxxxxxxx> <94943cf143035aa7adbe.1317823855@xxxxxxxxxxxx> <4E8D88CE0200007800059A1A@xxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Thu, 2011-10-06 at 09:54 +0100, Jan Beulich wrote:
> >>> On 05.10.11 at 16:10, Olaf Hering <olaf@xxxxxxxxx> wrote:
> > linux-2.6.18: xen/pv-on-hvm kexec: prevent crash in xenwatch_thread() when 
> > stale watch events arrive
> > 
> > commit c4c303c7c5679b4b368e12f41124aee29c325b76
> > 
> > During repeated kexec boots xenwatch_thread() can crash because
> > xenbus_watch->callback is cleared by xenbus_watch_path() if a node/token
> > combo for a new watch happens to match an already registered watch from
> > an old kernel.  In this case xs_watch returns -EEXISTS, then
> > register_xenbus_watch() does not remove the to-be-registered watch from
> > the list of active watches but returns the -EEXISTS to the caller
> > anyway.
> > 
> > Because the watch is still active in xenstored it will cause an event
> > which will arrive in the new kernel. process_msg() will find the
> > encapsulated struct xenbus_watch in its list of registered watches and
> > puts the "empty" watch handle in the queue for xenwatch_thread().
> > xenwatch_thread() then calls ->callback which was cleared earlier by
> > xenbus_watch_path().
> > 
> > To prevent that crash in a guest running on an old xen toolstack remove
> > the special -EEXIST handling.
> > 
> > v2:
> >  - remove the EEXIST handing in register_xenbus_watch() instead of
> >    checking for ->callback in process_msg()
> > 
> > Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> > 
> > diff -r 86e85596d64b -r 94943cf14303 drivers/xen/xenbus/xenbus_xs.c
> > --- a/drivers/xen/xenbus/xenbus_xs.c
> > +++ b/drivers/xen/xenbus/xenbus_xs.c
> > @@ -656,8 +656,7 @@ int register_xenbus_watch(struct xenbus_
> >  
> >     err = xs_watch(watch->node, token);
> >  
> > -   /* Ignore errors due to multiple registration. */
> > -   if ((err != 0) && (err != -EEXIST)) {
> > +   if (err) {
> 
> While I committed the other two patches in this series, this one seems
> to have the potential for regressions (the comment and the checking for
> -EEXIST can be assumed to have been there for a reason - whether
> they became stale by now is not obvious),

Keir said earlier it wasn't correct:
http://marc.info/?l=xen-devel&m=131358786516831&w=2

Ian.

>  so I'd like to double check
> that you verified that there's no code path where
> register_xenbus_watch() could be called twice for the same watch.
> 
> One group of cases of concern are the watches registered from
> xenstore notifiers - these appears to be safe, but the fact that they
> get called just once is only implicitly derivable walking through the
> code. And that may break the moment xenstore becomes a restartable
> entity.
> 
> The other possibly problematic case is that of watches user mode
> can register through writing the xenbus device: Here the patch
> definitely changes behavior observable by user mode (a
> re-registration does not cancel an existing watch without this
> change).
> 
> Jan
> 
> >             spin_lock(&watches_lock);
> >             list_del(&watch->list);
> >             spin_unlock(&watches_lock);
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel