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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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