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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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), 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