From patchwork Wed Jul 8 13:27:01 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lukas Wunner X-Patchwork-Id: 11651679 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 2D6D3618 for ; Wed, 8 Jul 2020 13:43:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 200F6206E9 for ; Wed, 8 Jul 2020 13:43:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729679AbgGHNnJ (ORCPT ); Wed, 8 Jul 2020 09:43:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43096 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729450AbgGHNnJ (ORCPT ); Wed, 8 Jul 2020 09:43:09 -0400 X-Greylist: delayed 582 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Wed, 08 Jul 2020 06:43:09 PDT Received: from mailout3.hostsharing.net (mailout3.hostsharing.net [IPv6:2a01:4f8:150:2161:1:b009:f236:0]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 53F47C061A0B for ; Wed, 8 Jul 2020 06:43:09 -0700 (PDT) Received: from h08.hostsharing.net (h08.hostsharing.net [IPv6:2a01:37:1000::53df:5f1c:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.hostsharing.net", Issuer "COMODO RSA Domain Validation Secure Server CA" (not verified)) by mailout3.hostsharing.net (Postfix) with ESMTPS id 34935101E69A5; Wed, 8 Jul 2020 15:33:23 +0200 (CEST) Received: from localhost (unknown [87.130.102.138]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by h08.hostsharing.net (Postfix) with ESMTPSA id C340F6106EC8; Wed, 8 Jul 2020 15:33:22 +0200 (CEST) X-Mailbox-Line: From de88a23a6fe0ef70f7cfd13c8aea9ab51b4edab6 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Lukas Wunner Date: Wed, 8 Jul 2020 15:27:01 +0200 Subject: [PATCH 1/3] driver core: Avoid binding drivers to dead devices To: Greg Kroah-Hartman Cc: "Rafael J. Wysocki" , Dan Williams , Geert Uytterhoeven , Pantelis Antoniou , Alexander Duyck , Mark Brown , linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Sender: linux-spi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org Commit 3451a495ef24 ("driver core: Establish order of operations for device_add and device_del via bitflag") sought to prevent asynchronous driver binding to a device which is being removed. It added a per-device "dead" flag which is checked in the following code paths: * asynchronous binding in __driver_attach_async_helper() * synchronous binding in device_driver_attach() * asynchronous binding in __device_attach_async_helper() It did *not* check the flag upon: * synchronous binding in __device_attach() However __device_attach() may also be called asynchronously from: deferred_probe_work_func() bus_probe_device() device_initial_probe() __device_attach() So if the commit's intention was to check the "dead" flag in all asynchronous code paths, then a check is also necessary in __device_attach(). Add the missing check. Fixes: 3451a495ef24 ("driver core: Establish order of operations for device_add and device_del via bitflag") Signed-off-by: Lukas Wunner Cc: stable@vger.kernel.org # v5.1+ Cc: Alexander Duyck --- drivers/base/dd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 9a1d940342ac4..31c668651e824 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -848,7 +848,9 @@ static int __device_attach(struct device *dev, bool allow_async) int ret = 0; device_lock(dev); - if (dev->driver) { + if (dev->p->dead) { + goto out_unlock; + } else if (dev->driver) { if (device_is_bound(dev)) { ret = 1; goto out_unlock; From patchwork Wed Jul 8 13:27:02 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lukas Wunner X-Patchwork-Id: 11651667 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 9D8E714DD for ; Wed, 8 Jul 2020 13:36:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 890B32065F for ; Wed, 8 Jul 2020 13:36:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729736AbgGHNgS (ORCPT ); Wed, 8 Jul 2020 09:36:18 -0400 Received: from mailout2.hostsharing.net ([83.223.78.233]:35203 "EHLO mailout2.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729732AbgGHNgR (ORCPT ); Wed, 8 Jul 2020 09:36:17 -0400 X-Greylist: delayed 373 seconds by postgrey-1.27 at vger.kernel.org; Wed, 08 Jul 2020 09:36:16 EDT Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.hostsharing.net", Issuer "COMODO RSA Domain Validation Secure Server CA" (not verified)) by mailout2.hostsharing.net (Postfix) with ESMTPS id 3522A10189B24; Wed, 8 Jul 2020 15:36:15 +0200 (CEST) Received: from localhost (pd95be530.dip0.t-ipconnect.de [217.91.229.48]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by h08.hostsharing.net (Postfix) with ESMTPSA id E7E796106EC8; Wed, 8 Jul 2020 15:36:14 +0200 (CEST) X-Mailbox-Line: From bf185285172a7b127424ac22fa42811eb2081cd4 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Lukas Wunner Date: Wed, 8 Jul 2020 15:27:02 +0200 Subject: [PATCH 2/3] driver core: Use rwsem for kill_device() serialization To: Greg Kroah-Hartman Cc: "Rafael J. Wysocki" , Dan Williams , Geert Uytterhoeven , Pantelis Antoniou , Alexander Duyck , Mark Brown , linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Sender: linux-spi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org kill_device() is currently serialized with driver probing by way of the device_lock(). We're about to serialize it with device_add() as well to prevent addition of children below a device which is going away. However the parent's device_lock() cannot be taken by device_add() lest deadlocks occur: Addition of the parent may result in addition of children (as is the case with SPI controllers) and device_add() already takes the device_lock through the call to bus_probe_device() -> device_initial_probe() -> __device_attach(). Avoid such deadlocks by introducing an rw_semaphore whose specific purpose is to serialize kill_device() with other parts of the kernel. Use an rw_semaphore instead of a mutex because the latter would preclude concurrent driver probing of multiple children below the same parent. The semaphore is acquired for writing when declaring a device dead and otherwise only acquired for reading. It is private to the driver core, obviating the need to acquire a lock when calling kill_device(), as is currently done in nvdimm's nd_device_unregister() and device_del(). An alternative approach would be to convert the device_lock() itself to an rw_semaphore (instead of a mutex). Signed-off-by: Lukas Wunner Cc: Dan Williams Cc: Geert Uytterhoeven Cc: Pantelis Antoniou Cc: Alexander Duyck --- drivers/base/base.h | 2 ++ drivers/base/core.c | 33 +++++++++++++++++++-------------- drivers/base/dd.c | 8 ++++++++ drivers/nvdimm/bus.c | 8 +------- 4 files changed, 30 insertions(+), 21 deletions(-) diff --git a/drivers/base/base.h b/drivers/base/base.h index 95c22c0f90360..7e71a1d262ef6 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -79,6 +79,7 @@ struct driver_private { * @async_driver - pointer to device driver awaiting probe via async_probe * @device - pointer back to the struct device that this structure is * associated with. + * @dead_sem - semaphore taken when declaring the device @dead. * @dead - This device is currently either in the process of or has been * removed from the system. Any asynchronous events scheduled for this * device should exit without taking any action. @@ -94,6 +95,7 @@ struct device_private { struct list_head deferred_probe; struct device_driver *async_driver; struct device *device; + struct rw_semaphore dead_sem; u8 dead:1; }; #define to_device_private_parent(obj) \ diff --git a/drivers/base/core.c b/drivers/base/core.c index 67d39a90b45c7..057da42b1a660 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2526,6 +2526,7 @@ static int device_private_init(struct device *dev) klist_init(&dev->p->klist_children, klist_children_get, klist_children_put); INIT_LIST_HEAD(&dev->p->deferred_probe); + init_rwsem(&dev->p->dead_sem); return 0; } @@ -2780,21 +2781,27 @@ void put_device(struct device *dev) } EXPORT_SYMBOL_GPL(put_device); +/** + * kill_device - declare device dead + * @dev: device in question + * + * Declare @dev dead to prevent it from binding to a driver. + * Return true if it was killed or false if it was already dead. + */ bool kill_device(struct device *dev) { - /* - * Require the device lock and set the "dead" flag to guarantee that - * the update behavior is consistent with the other bitfields near - * it and that we cannot have an asynchronous probe routine trying - * to run while we are tearing out the bus/class/sysfs from - * underneath the device. - */ - lockdep_assert_held(&dev->mutex); + bool killed; - if (dev->p->dead) - return false; - dev->p->dead = true; - return true; + down_write(&dev->p->dead_sem); + if (dev->p->dead) { + killed = false; + } else { + dev->p->dead = true; + killed = true; + } + up_write(&dev->p->dead_sem); + + return killed; } EXPORT_SYMBOL_GPL(kill_device); @@ -2817,9 +2824,7 @@ void device_del(struct device *dev) struct kobject *glue_dir = NULL; struct class_interface *class_intf; - device_lock(dev); kill_device(dev); - device_unlock(dev); if (dev->fwnode && dev->fwnode->dev == dev) dev->fwnode->dev = NULL; diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 31c668651e824..9353d811cce83 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -817,6 +817,7 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie) }; device_lock(dev); + down_read(&dev->p->dead_sem); /* * Check if device has already been removed or claimed. This may @@ -838,6 +839,7 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie) if (dev->parent) pm_runtime_put(dev->parent); out_unlock: + up_read(&dev->p->dead_sem); device_unlock(dev); put_device(dev); @@ -848,6 +850,7 @@ static int __device_attach(struct device *dev, bool allow_async) int ret = 0; device_lock(dev); + down_read(&dev->p->dead_sem); if (dev->p->dead) { goto out_unlock; } else if (dev->driver) { @@ -893,6 +896,7 @@ static int __device_attach(struct device *dev, bool allow_async) pm_runtime_put(dev->parent); } out_unlock: + up_read(&dev->p->dead_sem); device_unlock(dev); return ret; } @@ -967,6 +971,7 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) int ret = 0; __device_driver_lock(dev, dev->parent); + down_read(&dev->p->dead_sem); /* * If device has been removed or someone has already successfully @@ -975,6 +980,7 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) if (!dev->p->dead && !dev->driver) ret = driver_probe_device(drv, dev); + up_read(&dev->p->dead_sem); __device_driver_unlock(dev, dev->parent); return ret; @@ -987,6 +993,7 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) int ret = 0; __device_driver_lock(dev, dev->parent); + down_read(&dev->p->dead_sem); drv = dev->p->async_driver; @@ -997,6 +1004,7 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) if (!dev->p->dead && !dev->driver) ret = driver_probe_device(drv, dev); + up_read(&dev->p->dead_sem); __device_driver_unlock(dev, dev->parent); dev_dbg(dev, "driver %s async attach completed: %d\n", drv->name, ret); diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index 09087c38fabdc..35e069c69386a 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -559,8 +559,6 @@ EXPORT_SYMBOL(nd_device_register); void nd_device_unregister(struct device *dev, enum nd_async_mode mode) { - bool killed; - switch (mode) { case ND_ASYNC: /* @@ -584,11 +582,7 @@ void nd_device_unregister(struct device *dev, enum nd_async_mode mode) * or otherwise let the async path handle it if the * unregistration was already queued. */ - nd_device_lock(dev); - killed = kill_device(dev); - nd_device_unlock(dev); - - if (!killed) + if (!kill_device(dev)) return; nd_synchronize(); From patchwork Wed Jul 8 13:27:03 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lukas Wunner X-Patchwork-Id: 11651681 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id A2EB014B7 for ; Wed, 8 Jul 2020 13:44:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 92EB520720 for ; Wed, 8 Jul 2020 13:44:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729450AbgGHNoH (ORCPT ); Wed, 8 Jul 2020 09:44:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43248 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729411AbgGHNoH (ORCPT ); Wed, 8 Jul 2020 09:44:07 -0400 X-Greylist: delayed 366 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Wed, 08 Jul 2020 06:44:06 PDT Received: from mailout1.hostsharing.net (mailout1.hostsharing.net [IPv6:2a01:37:1000::53df:5fcc:0]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EDD4AC061A0B for ; Wed, 8 Jul 2020 06:44:06 -0700 (PDT) Received: from h08.hostsharing.net (h08.hostsharing.net [IPv6:2a01:37:1000::53df:5f1c:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.hostsharing.net", Issuer "COMODO RSA Domain Validation Secure Server CA" (not verified)) by mailout1.hostsharing.net (Postfix) with ESMTPS id 14E1010190FA4; Wed, 8 Jul 2020 15:37:57 +0200 (CEST) Received: from localhost (pd95be530.dip0.t-ipconnect.de [217.91.229.48]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by h08.hostsharing.net (Postfix) with ESMTPSA id B1E906106EC8; Wed, 8 Jul 2020 15:37:56 +0200 (CEST) X-Mailbox-Line: From f2d349b5ba67b5ca70cb19577725167642eb69c5 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Lukas Wunner Date: Wed, 8 Jul 2020 15:27:03 +0200 Subject: [PATCH 3/3] driver core: Avoid adding children below a dead parent To: Greg Kroah-Hartman Cc: "Rafael J. Wysocki" , Dan Williams , Geert Uytterhoeven , Pantelis Antoniou , Alexander Duyck , Mark Brown , linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Sender: linux-spi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org If CONFIG_OF_DYNAMIC or CONFIG_ACPI is enabled, SPI devices may be added below a controller at runtime by a DeviceTree overlay or DSDT patch. But there are no precautions to prevent adding a device below a controller that's being removed. This seems like something that should be guarded against in the driver core because it's not specific to SPI: Adding a child below a parent that's going away seems like a bad idea regardless of the bus type. Take advantage of kill_device() which was added by commit 00289cd87676 ("drivers/base: Introduce kill_device()"), call it upon removal of an SPI controller and teach the driver core to refuse device addition below a killed parent. To make this race-free, device_add() needs to take the parent's dead_sem before checking its "dead" flag and until the child device has been added to the parent's klist_children. Signed-off-by: Lukas Wunner Cc: Dan Williams Cc: Geert Uytterhoeven Cc: Pantelis Antoniou Cc: Alexander Duyck Reported-by: kernel test robot --- drivers/base/core.c | 18 ++++++++++++++++-- drivers/spi/spi.c | 3 +++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/base/core.c b/drivers/base/core.c index 057da42b1a660..1d4e39696f996 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -2597,6 +2597,14 @@ int device_add(struct device *dev) pr_debug("device: '%s': %s\n", dev_name(dev), __func__); parent = get_device(dev->parent); + if (parent) { + down_read(&parent->p->dead_sem); + if (parent->p->dead) { + error = -ENODEV; + goto parent_error; + } + } + kobj = get_device_parent(dev, parent); if (IS_ERR(kobj)) { error = PTR_ERR(kobj); @@ -2679,9 +2687,11 @@ int device_add(struct device *dev) } bus_probe_device(dev); - if (parent) + if (parent) { klist_add_tail(&dev->p->knode_parent, &parent->p->klist_children); + up_read(&parent->p->dead_sem); + } if (dev->class) { mutex_lock(&dev->class->p->mutex); @@ -2722,6 +2732,8 @@ int device_add(struct device *dev) Error: cleanup_glue_dir(dev, glue_dir); parent_error: + if (parent) + up_read(&parent->p->dead_sem); put_device(parent); name_error: kfree(dev->p); @@ -2785,7 +2797,9 @@ EXPORT_SYMBOL_GPL(put_device); * kill_device - declare device dead * @dev: device in question * - * Declare @dev dead to prevent it from binding to a driver. + * Declare @dev dead to prevent it from binding to a driver and + * to prevent addition of children. + * * Return true if it was killed or false if it was already dead. */ bool kill_device(struct device *dev) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8158e281f3540..005eca4bae089 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2764,6 +2764,9 @@ void spi_unregister_controller(struct spi_controller *ctlr) struct spi_controller *found; int id = ctlr->bus_num; + /* Prevent addition of new children, then remove existing ones */ + if (IS_ENABLED(CONFIG_OF_DYNAMIC) || IS_ENABLED(CONFIG_ACPI)) + kill_device(&ctlr->dev); device_for_each_child(&ctlr->dev, NULL, __unregister); /* First make sure that this controller was ever added */