[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-4.16] tools/xenstored: Ignore domain we were unable to restore


  • To: Julien Grall <julien@xxxxxxx>
  • From: Luca Fancellu <luca.fancellu@xxxxxxx>
  • Date: Thu, 21 Oct 2021 08:07:23 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=PIkQPWZwwrCJSTFe4KSt2/HnGoJQ9yChM5yBS1Qd+1s=; b=cRQWvMYyXzGdRfAURP9PoYqOvu3OdLPGCmox/k8a9+Ws+5c7RMAxSE1ruSBk8wPVqWoJlieYj850Mp8SmxFmoZsM5dAurE0LE10cOIsqM9OGGnSbOse/qOqEgpu5urOQuRqSTbAvl6zVG9+HwVxSw6ZFtKQnL/H6IgAHGDgFXWYdymSm4PhE9Cd6Ar9lb5dyJHmmw0OTnHQiicyJwBrYb+cykB+K7nYyY2ddfAHddODyLGMR+/wXpN0AKC9+rcfhlXYSjEoZevmnzgbeB1EeO/Kig6W70ivFa/17NnF5DGSRGF7KZhDMql4PLmpCbxseDANoi7Ysqj6oD/tFVj+5PA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZAwww46x4F4Asy5GYP62ANIWwk5Km1ugnhYQfCL9UeWobvuoh+8jn8VTcFgSZsIC8tECLCuRtKvuN5iWdjrUYY+++FXl7snNpga21b9viQVI0nJUa4yPQp5eMDaKFT3rZe5Mw5alE5ZqnZt2GWqvvjYJIDcAJvCtECM9DHAevDSOLjdAWQkIt8AhFzKKRI5bh/C5njTyw7+/RL7yse7QMIzeMGSYxXqMhn5u4gz9VDcdFnx2JP06yvnsKnMnO3q25u2XJEjH4doNkijyjYFsTx6j2/rQSdpz4AniDOKIMqBQeq0FGsyRp5GXKrY+w0X5xYNxXBcSLVXkKZvW6zP87Q==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Julien Grall <jgrall@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>
  • Delivery-date: Thu, 21 Oct 2021 07:07:49 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;


> On 20 Oct 2021, at 15:45, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Commit 939775cfd3 "handle dying domains in live update" was meant to
> handle gracefully dying domain. However, the @releaseDomain watch
> will end up to be sent as soon as we finished to restore Xenstored
> state.
> 
> This may be before Xen reports the domain to be dying (such as if
> the guest decided to revoke access to the xenstore page). Consequently
> daemon like xenconsoled will not clean-up the domain and it will be
> left as a zombie.
> 
> To avoid the problem, mark the connection as ignored. This also
> requires to tweak conn_can_write() and conn_can_read() to prevent
> dereferencing a NULL pointer (the interface will not mapped).
> 
> The check conn->is_ignored was originally added after the callbacks
> because the helpers for a socket connection may close the fd. However,
> ignore_connection() will close a socket connection directly. So it is
> fine to do the re-order.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>

> 
> ---
> 
> This issue was originally found when developping commit 939775cfd3.
> Unfortunately, due to some miscommunication the wrong patch was sent
> upstream. The approach is re-using the one we discussed back then.
> 
> This was tested by modifying Linux to revoke the Xenstore grant during
> boot. Without this patch, the domain will be left after Live-Update. Note
> that on a basic setup (only xenconsoled and xl watch @releaseDomain),
> the domain may be cleaned on the next domain shutdown/start.
> 
> Xenstore Live-Update is so far a tech preview feature. But I would still
> like to request this patch to be included in 4.16 as this was meant to
> be part of the original work.
> ---
> tools/xenstore/xenstored_core.c   |  8 ++++----
> tools/xenstore/xenstored_core.h   |  1 +
> tools/xenstore/xenstored_domain.c | 21 ++++++++++++---------
> 3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 0d4c73d6e20c..91d093a12ea6 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -338,10 +338,10 @@ static int destroy_conn(void *_conn)
> 
> static bool conn_can_read(struct connection *conn)
> {
> -     if (!conn->funcs->can_read(conn))
> +     if (conn->is_ignored)
>               return false;
> 
> -     if (conn->is_ignored)
> +     if (!conn->funcs->can_read(conn))
>               return false;
> 
>       /*
> @@ -356,7 +356,7 @@ static bool conn_can_read(struct connection *conn)
> 
> static bool conn_can_write(struct connection *conn)
> {
> -     return conn->funcs->can_write(conn) && !conn->is_ignored;
> +     return !conn->is_ignored && conn->funcs->can_write(conn);
> }
> 
> /* This function returns index inside the array if succeed, -1 if fail */
> @@ -1466,7 +1466,7 @@ static struct {
>  *
>  * All watches, transactions, buffers will be freed.
>  */
> -static void ignore_connection(struct connection *conn)
> +void ignore_connection(struct connection *conn)
> {
>       struct buffered_data *out, *tmp;
> 
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index 258f6ff38279..07d861d92499 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -206,6 +206,7 @@ struct node *read_node(struct connection *conn, const 
> void *ctx,
> 
> struct connection *new_connection(const struct interface_funcs *funcs);
> struct connection *get_connection_by_id(unsigned int conn_id);
> +void ignore_connection(struct connection *conn);
> void check_store(void);
> void corrupt(struct connection *conn, const char *fmt, ...);
> 
> diff --git a/tools/xenstore/xenstored_domain.c 
> b/tools/xenstore/xenstored_domain.c
> index 47e9107c144e..d03c7d93a9e7 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -268,14 +268,7 @@ void check_domains(void)
>                               domain->shutdown = true;
>                               notify = 1;
>                       }
> -                     /*
> -                      * On Restore, we may have been unable to remap the
> -                      * interface and the port. As we don't know whether
> -                      * this was because of a dying domain, we need to
> -                      * check if the interface and port are still valid.
> -                      */
> -                     if (!dominfo.dying && domain->port &&
> -                         domain->interface)
> +                     if (!dominfo.dying)
>                               continue;
>               }
>               if (domain->conn) {
> @@ -1303,6 +1296,17 @@ void read_state_connection(const void *ctx, const void 
> *state)
>               if (!domain)
>                       barf("domain allocation error");
> 
> +             conn = domain->conn;
> +
> +             /*
> +              * We may not have been able to restore the domain (for
> +              * instance because it revoked the Xenstore grant). We need
> +              * to keep it around to send @releaseDomain when it is
> +              * dead. So mark it as ignored.
> +              */
> +             if (!domain->port || !domain->interface)
> +                     ignore_connection(conn);
> +
>               if (sc->spec.ring.tdomid != DOMID_INVALID) {
>                       tdomain = find_or_alloc_domain(ctx,
>                                                      sc->spec.ring.tdomid);
> @@ -1311,7 +1315,6 @@ void read_state_connection(const void *ctx, const void 
> *state)
>                       talloc_reference(domain->conn, tdomain->conn);
>                       domain->conn->target = tdomain->conn;
>               }
> -             conn = domain->conn;
>       }
> 
>       conn->conn_id = sc->conn_id;
> -- 
> 2.32.0
> 
> 




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.