From patchwork Wed Nov 15 18:25:44 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Robin Murphy X-Patchwork-Id: 165544 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b909:0:b0:403:3b70:6f57 with SMTP id t9csp2729162vqg; Wed, 15 Nov 2023 10:26:20 -0800 (PST) X-Google-Smtp-Source: AGHT+IHJGLpoN80U2O4De11C9j5bXKyqo5770M8XRmeXaN0x1gz8J4nXghoWFd89MIiRVIOMMp7M X-Received: by 2002:a05:6a00:198d:b0:6c6:af58:c9f5 with SMTP id d13-20020a056a00198d00b006c6af58c9f5mr15137183pfl.28.1700072780596; Wed, 15 Nov 2023 10:26:20 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700072780; cv=none; d=google.com; s=arc-20160816; b=proUPeenw2XK4t4jhzSwuLaV0JsFKJDQYyBNZaxGp48Qpqvv6cCKskDwMD987ihNYT l7R0hQFsfnPZ/H+OAKcdqJc66e9yU0/eXJoJLs/IWkO83l+fkolZlYsYk7kaPrr+KSl5 ITT5qhFNKSf6oIf2dC473ETAHBCA2J/dIxQjFKNengclnqGxhR0Zv9BYc2R3Ydpittn2 DIvKkZkm8ulQX31TcN9928nm7F5OaxdxRlOKLXZsUX27b46Lgwk3xfr/u+MEFiKdL1n4 07sP/x2pdBeJmNG9lq8MPorbXJMoE5u6NWaTBmsTOU+q2BD0MIU+HDFO/5w8LE9IV1p5 cnhg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=ls+zA1m7mDbmit4ndcr8TbtE1yYbCRpsXfhr0cMkMUQ=; fh=SJQl1H5iWO4H0jeXVRwhIT6D1j44vph6hQYkvva9SWM=; b=uFQvdX3zC1meqOLZ8s4GQNXZI8cjgRaq4PIBFkzY1vYJt2/yKt3XfylhoxhBX82l5f 6WF7HddgItzaT6OtUp9j1JevQmFMR70/zj9ru+g7on4bpH483Uvx8ift4wJhjkNRt8JV +sYHbGxIAYuZvvxQ80AvQ3XT1BOMcuca3+7Lyt6KeAKG3P1lSV7N+nWA4//CeqmHPwTt UgTqfoAmaDMYYKDjpXcrSPnlo3P8AJRqUIoeT8K+OUj0emMBIO95QOLp9jWzFc8HHR21 G6JTjnyRuMBzBvJ3+G4VLNnSQc/WXc2YKz1PD4njgIutiFVyT7527JNtTj2+Tt+HYQJR UbNw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from fry.vger.email (fry.vger.email. [23.128.96.38]) by mx.google.com with ESMTPS id c128-20020a633586000000b005b9a1494637si10593932pga.317.2023.11.15.10.26.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Nov 2023 10:26:20 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id D1503808BD9D; Wed, 15 Nov 2023 10:26:17 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232391AbjKOSZ7 (ORCPT + 29 others); Wed, 15 Nov 2023 13:25:59 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38532 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229630AbjKOSZ6 (ORCPT ); Wed, 15 Nov 2023 13:25:58 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 900EC1B8; Wed, 15 Nov 2023 10:25:54 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E4D47DA7; Wed, 15 Nov 2023 10:26:39 -0800 (PST) Received: from e121345-lin.cambridge.arm.com (e121345-lin.cambridge.arm.com [10.1.196.40]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id CC0653F641; Wed, 15 Nov 2023 10:25:52 -0800 (PST) From: Robin Murphy To: joro@8bytes.org, will@kernel.org Cc: iommu@lists.linux.dev, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, rafael@kernel.org, lenb@kernel.org, lpieralisi@kernel.org, andre.draszik@linaro.org, quic_zhenhuah@quicinc.com, jgg@nvidia.com Subject: [PATCH] iommu: Avoid more races around device probe Date: Wed, 15 Nov 2023 18:25:44 +0000 Message-Id: <16f433658661d7cadfea51e7c65da95826112a2b.1700071477.git.robin.murphy@arm.com> X-Mailer: git-send-email 2.39.2.101.g768bb238c484.dirty MIME-Version: 1.0 X-Spam-Status: No, score=-0.8 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Wed, 15 Nov 2023 10:26:17 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782655515898427438 X-GMAIL-MSGID: 1782655515898427438 It turns out there are more subtle races beyond just the main part of __iommu_probe_device() itself running in parallel - the dev_iommu_free() on the way out of an unsuccessful probe can still manage to trip up concurrent accesses to a device's fwspec. Thus, extend the scope of iommu_probe_device_lock() to also serialise fwspec creation and initial retrieval. Reported-by: Zhenhua Huang Link: https://lore.kernel.org/linux-iommu/e2e20e1c-6450-4ac5-9804-b0000acdf7de@quicinc.com/ Fixes: 01657bc14a39 ("iommu: Avoid races around device probe") Signed-off-by: Robin Murphy Reviewed-by: André Draszik Tested-by: André Draszik (using continuous Acked-by: Greg Kroah-Hartman --- This is my idea of a viable fix, since it does not need a 700-line diffstat to make the code do what it was already *trying* to do anyway. This stuff should fundamentally not be hanging off driver probe in the first place, so I'd rather get on with removing the underlying brokenness than waste time and effort polishing it any further. drivers/acpi/scan.c | 7 ++++++- drivers/iommu/iommu.c | 20 ++++++++++---------- drivers/iommu/of_iommu.c | 12 +++++++++--- include/linux/iommu.h | 1 + 4 files changed, 26 insertions(+), 14 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index fa5dd71a80fa..02bb2cce423f 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1568,17 +1568,22 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev, int err; const struct iommu_ops *ops; + /* Serialise to make dev->iommu stable under our potential fwspec */ + mutex_lock(&iommu_probe_device_lock); /* * If we already translated the fwspec there is nothing left to do, * return the iommu_ops. */ ops = acpi_iommu_fwspec_ops(dev); - if (ops) + if (ops) { + mutex_unlock(&iommu_probe_device_lock); return ops; + } err = iort_iommu_configure_id(dev, id_in); if (err && err != -EPROBE_DEFER) err = viot_iommu_configure(dev); + mutex_unlock(&iommu_probe_device_lock); /* * If we have reason to believe the IOMMU driver missed the initial diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index f17a1113f3d6..e0c962648dde 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -485,11 +485,12 @@ static void iommu_deinit_device(struct device *dev) dev_iommu_free(dev); } +DEFINE_MUTEX(iommu_probe_device_lock); + static int __iommu_probe_device(struct device *dev, struct list_head *group_list) { const struct iommu_ops *ops = dev->bus->iommu_ops; struct iommu_group *group; - static DEFINE_MUTEX(iommu_probe_device_lock); struct group_device *gdev; int ret; @@ -502,17 +503,15 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list * probably be able to use device_lock() here to minimise the scope, * but for now enforcing a simple global ordering is fine. */ - mutex_lock(&iommu_probe_device_lock); + lockdep_assert_held(&iommu_probe_device_lock); /* Device is probed already if in a group */ - if (dev->iommu_group) { - ret = 0; - goto out_unlock; - } + if (dev->iommu_group) + return 0; ret = iommu_init_device(dev, ops); if (ret) - goto out_unlock; + return ret; group = dev->iommu_group; gdev = iommu_group_alloc_device(group, dev); @@ -548,7 +547,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list list_add_tail(&group->entry, group_list); } mutex_unlock(&group->mutex); - mutex_unlock(&iommu_probe_device_lock); if (dev_is_pci(dev)) iommu_dma_set_pci_32bit_workaround(dev); @@ -562,8 +560,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list iommu_deinit_device(dev); mutex_unlock(&group->mutex); iommu_group_put(group); -out_unlock: - mutex_unlock(&iommu_probe_device_lock); return ret; } @@ -573,7 +569,9 @@ int iommu_probe_device(struct device *dev) const struct iommu_ops *ops; int ret; + mutex_lock(&iommu_probe_device_lock); ret = __iommu_probe_device(dev, NULL); + mutex_unlock(&iommu_probe_device_lock); if (ret) return ret; @@ -1822,7 +1820,9 @@ static int probe_iommu_group(struct device *dev, void *data) struct list_head *group_list = data; int ret; + mutex_lock(&iommu_probe_device_lock); ret = __iommu_probe_device(dev, group_list); + mutex_unlock(&iommu_probe_device_lock); if (ret == -ENODEV) ret = 0; diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 157b286e36bf..c25b4ae6aeee 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -112,16 +112,20 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, const u32 *id) { const struct iommu_ops *ops = NULL; - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); + struct iommu_fwspec *fwspec; int err = NO_IOMMU; if (!master_np) return NULL; + /* Serialise to make dev->iommu stable under our potential fwspec */ + mutex_lock(&iommu_probe_device_lock); + fwspec = dev_iommu_fwspec_get(dev); if (fwspec) { - if (fwspec->ops) + if (fwspec->ops) { + mutex_unlock(&iommu_probe_device_lock); return fwspec->ops; - + } /* In the deferred case, start again from scratch */ iommu_fwspec_free(dev); } @@ -155,6 +159,8 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, fwspec = dev_iommu_fwspec_get(dev); ops = fwspec->ops; } + mutex_unlock(&iommu_probe_device_lock); + /* * If we have reason to believe the IOMMU driver missed the initial * probe for dev, replay it to get things in order. diff --git a/include/linux/iommu.h b/include/linux/iommu.h index ec289c1016f5..6291aa7b079b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -845,6 +845,7 @@ static inline void dev_iommu_priv_set(struct device *dev, void *priv) dev->iommu->priv = priv; } +extern struct mutex iommu_probe_device_lock; int iommu_probe_device(struct device *dev); int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);