From nobody Sat May 18 04:13:30 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.xenproject.org designates 192.237.175.120 as permitted sender) client-ip=192.237.175.120; envelope-from=xen-devel-bounces@lists.xenproject.org; helo=lists.xenproject.org; Authentication-Results: mx.zohomail.com; spf=pass (zohomail.com: domain of lists.xenproject.org designates 192.237.175.120 as permitted sender) smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Return-Path: Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) by mx.zohomail.com with SMTPS id 1653504189101418.3362338519647; Wed, 25 May 2022 11:43:09 -0700 (PDT) Received: from list by lists.xenproject.org with outflank-mailman.337338.561896 (Exim 4.92) (envelope-from ) id 1ntvxd-0002YH-2B; Wed, 25 May 2022 18:42:21 +0000 Received: by outflank-mailman (output) from mailman id 337338.561896; Wed, 25 May 2022 18:42:21 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1ntvxc-0002YA-VX; Wed, 25 May 2022 18:42:20 +0000 Received: by outflank-mailman (input) for mailman id 337338; Wed, 25 May 2022 18:42:20 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1ntvxc-0002Y4-6i for xen-devel@lists.xenproject.org; Wed, 25 May 2022 18:42:20 +0000 Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id 65de278b-dc5a-11ec-837f-e5687231ffcc; Wed, 25 May 2022 20:42:16 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 324F55C00F1; Wed, 25 May 2022 14:42:15 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Wed, 25 May 2022 14:42:15 -0400 Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 25 May 2022 14:42:14 -0400 (EDT) X-Outflank-Mailman: Message body and most headers restored to incoming version X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 65de278b-dc5a-11ec-837f-e5687231ffcc DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= invisiblethingslab.com; h=cc:cc:content-transfer-encoding:date :date:from:from:in-reply-to:message-id:mime-version:reply-to :sender:subject:subject:to:to; s=fm1; t=1653504135; x= 1653590535; bh=IzC1Lf6nigMRWiB9U/JcQwb9cl+2WkqbqCQ8B2xwhgQ=; b=s 6ZP5jmjsZIoyBGP4epU3kxEFo1+la+txzb8Lwu9EpCUdo67NzNiBGyMBKb2qNVXG 580T5/EfIsj1IAM5AZHSBDarEsdGz0fVu5O4CoLnNlqksMJeGR0tdgEHx/9t+I/t xJz6gvWdCX7GS/IFCRJdoUAlU3oaJ832DWq8AGOgE6+p8iOtkYuto5Vx2QRBUkiU VCI2g6ea6rnz6Q+CftOvlp938vebEW9kP/SI2kN1siwxSycKilfpam1mbU6va3+4 2WBN6val8LlLbtZTH6nfuwHU+gFNNHPYesjh4jLbhV9nhcxdbqbKWndeX/gTb+Lu nfR4VzcYpxdUaWLNx6apQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding:date:date :feedback-id:feedback-id:from:from:in-reply-to:message-id :mime-version:reply-to:sender:subject:subject:to:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t= 1653504135; x=1653590535; bh=IzC1Lf6nigMRWiB9U/JcQwb9cl+2WkqbqCQ 8B2xwhgQ=; b=hgYCk+9L+3wKSB7dO0pFs73z2RoYenuk8rByPFtDwl2EIrJz+qH loluingXVrThMudBibrCFJNkkCWTi/TqNNVWpreTArOfWUUpNXg8jBSubnL4qSUp UwlMYuqvgHrC7bHZkzgtEDxBYwPr7Ad/5a2TNnaf1cSkheqo/bR2tp02yG7C6Q+a hbU+YSP6ZCCrZzscu5Ej+d9aOWZtlkfprXaeM9mfce5A1LQENwp4913lA1FYhDxP 43sKka8D3ULAP6VFyOvPoSL6kGEpOsrNrUAsNrGbfRq2j+aWxUzKYCGh96BsWv8c 3K6ahDga19tUCb++gFPcfDpP0EwhFMTbxuQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrjeehgdduvdeiucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkofgggfestdekredtredttdenucfhrhhomhepffgvmhhiucfo rghrihgvucfqsggvnhhouhhruceouggvmhhisehinhhvihhsihgslhgvthhhihhnghhslh grsgdrtghomheqnecuggftrfgrthhtvghrnhepieejgedufeeukeeijedukefgueekvdeg iedtudefhfdtffehffeuveefvdfglefgnecuffhomhgrihhnpehgihhthhhusgdrtghomh enucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpeguvghm ihesihhnvhhishhisghlvghthhhinhhgshhlrggsrdgtohhm X-ME-Proxy: Feedback-ID: iac594737:Fastmail From: Demi Marie Obenour To: Boris Ostrovsky , Juergen Gross , Stefano Stabellini , Jennifer Herbert , =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Cc: Demi Marie Obenour , xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: [PATCH v2] xen/gntdev: Avoid blocking in unmap_grant_pages() Date: Wed, 25 May 2022 14:41:53 -0400 Message-Id: <20220525184153.6059-1-demi@invisiblethingslab.com> X-Mailer: git-send-email 2.35.3 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-ZM-MESSAGEID: 1653504191080100001 Content-Type: text/plain; charset="utf-8" unmap_grant_pages() currently waits for the pages to no longer be used. In https://github.com/QubesOS/qubes-issues/issues/7481, this lead to a deadlock against i915: i915 was waiting for gntdev's MMU notifier to finish, while gntdev was waiting for i915 to free its pages. I also believe this is responsible for various deadlocks I have experienced in the past. Avoid these problems by making unmap_grant_pages async. This requires making it return void, as any errors will not be available when the function returns. Fortunately, the only use of the return value is a WARN_ON(), which can be replaced by a WARN_ON when the error is detected. Additionally, a failed call will not prevent further calls from being made, but this is harmless. Because unmap_grant_pages is now async, the grant handle will be sent to INVALID_GRANT_HANDLE too late to prevent multiple unmaps of the same handle. Instead, a separate bool array is allocated for this purpose. This wastes memory, but stuffing this information in padding bytes is too fragile. Furthermore, it is necessary to grab a reference to the map before making the asynchronous call, and release the reference when the call returns. Fixes: 745282256c75 ("xen/gntdev: safely unmap grants in case they are stil= l in use") Cc: stable@vger.kernel.org Signed-off-by: Demi Marie Obenour --- drivers/xen/gntdev-common.h | 5 ++ drivers/xen/gntdev.c | 100 +++++++++++++++++++----------------- 2 files changed, 59 insertions(+), 46 deletions(-) diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h index 20d7d059dadb..a268cdb1f7bf 100644 --- a/drivers/xen/gntdev-common.h +++ b/drivers/xen/gntdev-common.h @@ -16,6 +16,7 @@ #include #include #include +#include =20 struct gntdev_dmabuf_priv; =20 @@ -56,6 +57,7 @@ struct gntdev_grant_map { struct gnttab_unmap_grant_ref *unmap_ops; struct gnttab_map_grant_ref *kmap_ops; struct gnttab_unmap_grant_ref *kunmap_ops; + bool *being_removed; struct page **pages; unsigned long pages_vm_start; =20 @@ -73,6 +75,9 @@ struct gntdev_grant_map { /* Needed to avoid allocation in gnttab_dma_free_pages(). */ xen_pfn_t *frames; #endif + + /* Needed to avoid allocation in __unmap_grant_pages */ + struct gntab_unmap_queue_data unmap_data; }; =20 struct gntdev_grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int co= unt, diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 59ffea800079..90bd2b5ef7dd 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -35,6 +35,7 @@ #include #include #include +#include =20 #include #include @@ -62,8 +63,8 @@ MODULE_PARM_DESC(limit, =20 static int use_ptemod; =20 -static int unmap_grant_pages(struct gntdev_grant_map *map, - int offset, int pages); +static void unmap_grant_pages(struct gntdev_grant_map *map, + int offset, int pages); =20 static struct miscdevice gntdev_miscdev; =20 @@ -120,6 +121,7 @@ static void gntdev_free_map(struct gntdev_grant_map *ma= p) kvfree(map->unmap_ops); kvfree(map->kmap_ops); kvfree(map->kunmap_ops); + kvfree(map->being_removed); kfree(map); } =20 @@ -140,10 +142,13 @@ struct gntdev_grant_map *gntdev_alloc_map(struct gntd= ev_priv *priv, int count, add->unmap_ops =3D kvmalloc_array(count, sizeof(add->unmap_ops[0]), GFP_KERNEL); add->pages =3D kvcalloc(count, sizeof(add->pages[0]), GFP_KERNEL); + add->being_removed =3D + kvcalloc(count, sizeof(add->being_removed[0]), GFP_KERNEL); if (NULL =3D=3D add->grants || NULL =3D=3D add->map_ops || NULL =3D=3D add->unmap_ops || - NULL =3D=3D add->pages) + NULL =3D=3D add->pages || + NULL =3D=3D add->being_removed) goto err; if (use_ptemod) { add->kmap_ops =3D kvmalloc_array(count, sizeof(add->kmap_ops[0]), @@ -349,79 +354,84 @@ int gntdev_map_grant_pages(struct gntdev_grant_map *m= ap) return err; } =20 -static int __unmap_grant_pages(struct gntdev_grant_map *map, int offset, - int pages) +static void __unmap_grant_pages_done(int result, + struct gntab_unmap_queue_data *data) { - int i, err =3D 0; - struct gntab_unmap_queue_data unmap_data; - - if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) { - int pgno =3D (map->notify.addr >> PAGE_SHIFT); - if (pgno >=3D offset && pgno < offset + pages) { - /* No need for kmap, pages are in lowmem */ - uint8_t *tmp =3D pfn_to_kaddr(page_to_pfn(map->pages[pgno])); - tmp[map->notify.addr & (PAGE_SIZE-1)] =3D 0; - map->notify.flags &=3D ~UNMAP_NOTIFY_CLEAR_BYTE; - } - } - - unmap_data.unmap_ops =3D map->unmap_ops + offset; - unmap_data.kunmap_ops =3D use_ptemod ? map->kunmap_ops + offset : NULL; - unmap_data.pages =3D map->pages + offset; - unmap_data.count =3D pages; - - err =3D gnttab_unmap_refs_sync(&unmap_data); - if (err) - return err; + unsigned int i; + struct gntdev_grant_map *map =3D data->data; + unsigned int offset =3D data->unmap_ops - map->unmap_ops; =20 - for (i =3D 0; i < pages; i++) { - if (map->unmap_ops[offset+i].status) - err =3D -EINVAL; + for (i =3D 0; i < data->count; i++) { + WARN_ON(map->unmap_ops[offset+i].status); pr_debug("unmap handle=3D%d st=3D%d\n", map->unmap_ops[offset+i].handle, map->unmap_ops[offset+i].status); map->unmap_ops[offset+i].handle =3D INVALID_GRANT_HANDLE; if (use_ptemod) { - if (map->kunmap_ops[offset+i].status) - err =3D -EINVAL; + WARN_ON(map->kunmap_ops[offset+i].status); pr_debug("kunmap handle=3D%u st=3D%d\n", map->kunmap_ops[offset+i].handle, map->kunmap_ops[offset+i].status); map->kunmap_ops[offset+i].handle =3D INVALID_GRANT_HANDLE; } } - return err; + + /* Release reference taken by __unmap_grant_pages */ + gntdev_put_map(NULL, map); } =20 -static int unmap_grant_pages(struct gntdev_grant_map *map, int offset, - int pages) +static void __unmap_grant_pages(struct gntdev_grant_map *map, int offset, + int pages) +{ + if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) { + int pgno =3D (map->notify.addr >> PAGE_SHIFT); + + if (pgno >=3D offset && pgno < offset + pages) { + /* No need for kmap, pages are in lowmem */ + uint8_t *tmp =3D pfn_to_kaddr(page_to_pfn(map->pages[pgno])); + + tmp[map->notify.addr & (PAGE_SIZE-1)] =3D 0; + map->notify.flags &=3D ~UNMAP_NOTIFY_CLEAR_BYTE; + } + } + + map->unmap_data.unmap_ops =3D map->unmap_ops + offset; + map->unmap_data.kunmap_ops =3D use_ptemod ? map->kunmap_ops + offset : NU= LL; + map->unmap_data.pages =3D map->pages + offset; + map->unmap_data.count =3D pages; + map->unmap_data.done =3D __unmap_grant_pages_done; + map->unmap_data.data =3D map; + refcount_inc(&map->users); /* to keep map alive during async call below */ + + gnttab_unmap_refs_async(&map->unmap_data); +} + +static void unmap_grant_pages(struct gntdev_grant_map *map, int offset, + int pages) { - int range, err =3D 0; + int range; =20 pr_debug("unmap %d+%d [%d+%d]\n", map->index, map->count, offset, pages); =20 /* It is possible the requested range will have a "hole" where we * already unmapped some of the grants. Only unmap valid ranges. */ - while (pages && !err) { - while (pages && - map->unmap_ops[offset].handle =3D=3D INVALID_GRANT_HANDLE) { + while (pages) { + while (pages && map->being_removed[offset]) { offset++; pages--; } range =3D 0; while (range < pages) { - if (map->unmap_ops[offset + range].handle =3D=3D - INVALID_GRANT_HANDLE) + if (map->being_removed[offset + range]) break; + map->being_removed[offset + range] =3D true; range++; } - err =3D __unmap_grant_pages(map, offset, range); + __unmap_grant_pages(map, offset, range); offset +=3D range; pages -=3D range; } - - return err; } =20 /* ------------------------------------------------------------------ */ @@ -473,7 +483,6 @@ static bool gntdev_invalidate(struct mmu_interval_notif= ier *mn, struct gntdev_grant_map *map =3D container_of(mn, struct gntdev_grant_map, notifier); unsigned long mstart, mend; - int err; =20 if (!mmu_notifier_range_blockable(range)) return false; @@ -494,10 +503,9 @@ static bool gntdev_invalidate(struct mmu_interval_noti= fier *mn, map->index, map->count, map->vma->vm_start, map->vma->vm_end, range->start, range->end, mstart, mend); - err =3D unmap_grant_pages(map, + unmap_grant_pages(map, (mstart - map->vma->vm_start) >> PAGE_SHIFT, (mend - mstart) >> PAGE_SHIFT); - WARN_ON(err); =20 return true; } --=20 Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab