From patchwork Thu May 11 10:55:43 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Roger_Pau_Monn=C3=A9?= X-Patchwork-Id: 9721189 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B8D0760387 for ; Thu, 11 May 2017 10:58:08 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B49C62865A for ; Thu, 11 May 2017 10:58:08 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A97BD2865D; Thu, 11 May 2017 10:58:08 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5E5CD2865A for ; Thu, 11 May 2017 10:58:07 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d8llC-0004uv-3U; Thu, 11 May 2017 10:55:54 +0000 Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d8ll9-0004u9-DU for xen-devel@lists.xenproject.org; Thu, 11 May 2017 10:55:51 +0000 Received: from [85.158.143.35] by server-7.bemta-6.messagelabs.com id BA/90-03620-63344195; Thu, 11 May 2017 10:55:50 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprJIsWRWlGSWpSXmKPExsXitHSDva6Zs0i kwetprBbft0xmcmD0OPzhCksAYxRrZl5SfkUCa8aVpjvsBdu1Kn7/vcvUwLhJrouRg0NCwF/i zM7SLkZODjYBHYmLc3eygYRFBFQkbu816GLk4mAWmMko8bu/lQUkLiwQJvHnQy1IOYuAqkTn3 6PsIDavgKXE642HmUBsCQE9ibcTXzCC2JwCVhJPnj5kAmkVAqrZ3mUCUS4ocXLmExYQm1lAU6 J1+292CFteonnrbGYQW0hAUaJ/3gO2CYx8s5C0zELSMgtJywJG5lWM6sWpRWWpRbrGeklFmek ZJbmJmTm6hgZmermpxcWJ6ak5iUnFesn5uZsYgUHGAAQ7GDv+OR1ilORgUhLlbf8jHCnEl5Sf UpmRWJwRX1Sak1p8iFGGg0NJgrfGUSRSSLAoNT21Ii0zBxjuMGkJDh4lEd5zDkBp3uKCxNziz HSI1ClGRSlxXh0noIQASCKjNA+uDRZjlxhlpYR5GYEOEeIpSC3KzSxBlX/FKM7BqCTM+x9kO0 9mXgnc9FdAi5mAFveD3MxbXJKIkJJqYNy+Uu+Q75ciz4VHdhS8eXFx5fMZzhLZv8vF5r4Qsws p8vS7oX07IDW3qVdNxr6YtcJWOcnL48abxZtOij+pXtl5Q0fpv+oOl6Oa9dO3cjl+id3Lzf7x tPUX87l7cgqm/ypszwmbMjG6mSdUuutnvsgh68vz2DaqHvSr3hpi9/aaEGPWsuNTvJVYijMSD bWYi4oTAbWT4wOsAgAA X-Env-Sender: prvs=29737f781=roger.pau@citrix.com X-Msg-Ref: server-7.tower-21.messagelabs.com!1494500148!67941479!2 X-Originating-IP: [66.165.176.63] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni42MyA9PiAzMDYwNDg=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 9.4.12; banners=-,-,- X-VirusChecked: Checked Received: (qmail 31352 invoked from network); 11 May 2017 10:55:49 -0000 Received: from smtp02.citrix.com (HELO SMTP02.CITRIX.COM) (66.165.176.63) by server-7.tower-21.messagelabs.com with RC4-SHA encrypted SMTP; 11 May 2017 10:55:49 -0000 X-IronPort-AV: E=Sophos;i="5.38,324,1491264000"; d="scan'208";a="431861656" From: Roger Pau Monne To: Date: Thu, 11 May 2017 11:55:43 +0100 Message-ID: <20170511105544.35197-2-roger.pau@citrix.com> X-Mailer: git-send-email 2.11.0 (Apple Git-81) In-Reply-To: <20170511105544.35197-1-roger.pau@citrix.com> References: <20170511105544.35197-1-roger.pau@citrix.com> MIME-Version: 1.0 Cc: Wei Liu , Julien Grall , Ian Jackson , Roger Pau Monne Subject: [Xen-devel] [PATCH v2 1/2] libxl/devd: fix a race with concurrent device addition/removal X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP Current code can free the libxl__device inside of the libxl__ddomain_device before the addition has finished if a removal happens while an addition is still in process: backend_watch_callback | v add_device | backend_watch_callback (async operation) | | v | remove_device | | | V | device_complete | (free libxl__device) v device_complete (deref libxl__device) Fix this by creating a temporary copy of the libxl__device, that's tracked by the GC of the nested async operation. This ensures that the libxl__device used by the async operations cannot be freed while being used. Signed-off-by: Roger Pau Monné Reported-by: Ian Jackson Reviewed-by: Wei Liu --- Cc: Ian Jackson Cc: Wei Liu Cc: Julien Grall --- tools/libxl/libxl_device.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c index 5e966762c6..cd4ad05a6f 100644 --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -1415,9 +1415,6 @@ static void device_complete(libxl__egc *egc, libxl__ao_device *aodev) libxl__device_action_to_string(aodev->action), aodev->rc ? "failed" : "succeed"); - if (aodev->action == LIBXL__DEVICE_ACTION_REMOVE) - free(aodev->dev); - libxl__nested_ao_free(aodev->ao); } @@ -1521,7 +1518,12 @@ static int add_device(libxl__egc *egc, libxl__ao *ao, GCNEW(aodev); libxl__prepare_ao_device(ao, aodev); - aodev->dev = dev; + /* + * Clone the libxl__device to avoid races if remove_device is called + * before the device addition has finished. + */ + GCNEW(aodev->dev); + *aodev->dev = *dev; aodev->action = LIBXL__DEVICE_ACTION_ADD; aodev->callback = device_complete; libxl__wait_device_connection(egc, aodev); @@ -1564,7 +1566,12 @@ static int remove_device(libxl__egc *egc, libxl__ao *ao, GCNEW(aodev); libxl__prepare_ao_device(ao, aodev); - aodev->dev = dev; + /* + * Clone the libxl__device to avoid races if there's a add_device + * running in parallel. + */ + GCNEW(aodev->dev); + *aodev->dev = *dev; aodev->action = LIBXL__DEVICE_ACTION_REMOVE; aodev->callback = device_complete; libxl__initiate_device_generic_remove(egc, aodev); @@ -1576,7 +1583,6 @@ static int remove_device(libxl__egc *egc, libxl__ao *ao, goto out; } libxl__device_destroy(gc, dev); - free(dev); /* Fall through to return > 0, no ao has been dispatched */ default: rc = 1; @@ -1597,7 +1603,7 @@ static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch, char *p, *path; const char *sstate, *sonline; int state, online, rc, num_devs; - libxl__device *dev = NULL; + libxl__device *dev; libxl__ddomain_device *ddev = NULL; libxl__ddomain_guest *dguest = NULL; bool free_ao = false; @@ -1625,7 +1631,7 @@ static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch, goto skip; online = atoi(sonline); - dev = libxl__zalloc(NOGC, sizeof(*dev)); + GCNEW(dev); rc = libxl__parse_backend_path(gc, path, dev); if (rc) goto skip; @@ -1659,7 +1665,8 @@ static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch, * to the list of active devices for a given guest. */ ddev = libxl__zalloc(NOGC, sizeof(*ddev)); - ddev->dev = dev; + ddev->dev = libxl__zalloc(NOGC, sizeof(*ddev->dev)); + *ddev->dev = *dev; LIBXL_SLIST_INSERT_HEAD(&dguest->devices, ddev, next); LOGD(DEBUG, dev->domid, "Added device %s to the list of active devices", path); @@ -1670,9 +1677,6 @@ static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch, /* * Removal of an active device, remove it from the list and * free it's data structures if they are no longer needed. - * - * The free of the associated libxl__device is left to the - * helper remove_device function. */ LIBXL_SLIST_REMOVE(&dguest->devices, ddev, libxl__ddomain_device, next); @@ -1682,6 +1686,7 @@ static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch, if (rc > 0) free_ao = true; + free(ddev->dev); free(ddev); /* If this was the last device in the domain, remove it from the list */ num_devs = dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks; @@ -1703,7 +1708,8 @@ static void backend_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch, skip: libxl__nested_ao_free(nested_ao); - free(dev); + if (ddev) + free(ddev->dev); free(ddev); free(dguest); return;