All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Linux ACPI <linux-acpi@vger.kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>, Hans de Goede <hdegoede@redhat.com>
Subject: [PATCH v2 5/5] ACPI: scan: Fix race related to dropping dependencies
Date: Wed, 16 Jun 2021 20:00:32 +0200	[thread overview]
Message-ID: <11783109.O9o76ZdvQC@kreacher> (raw)
In-Reply-To: <3070024.5fSG56mABF@kreacher>

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If acpi_add_single_object() runs concurrently with respect to
acpi_scan_clear_dep() which deletes a dependencies list entry where
the device being added is the consumer, the device's dep_unmet
counter may not be updated to reflect that change.

Namely, if the dependencies list entry is deleted right after
calling acpi_scan_dep_init() and before calling acpi_device_add(),
acpi_scan_clear_dep() will not find the device object corresponding
to the consumer device ACPI handle and it will not update its
dep_unmet counter to reflect the deletion of the list entry.
Consequently, the dep_unmet counter of the device will never
become zero going forward which may prevent it from being
completely enumerated.

To address this problem, modify acpi_add_single_object() to run
acpi_tie_acpi_dev(), to attach the ACPI device object created by it
to the corresponding ACPI namespace node, under acpi_dep_list_lock
along with acpi_scan_dep_init() whenever the latter is called.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---

-> v2:
   * Fix memory leak spotted by Hans.
   * Add the R-by tag from Hans.

---
 drivers/acpi/scan.c |   50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -657,16 +652,12 @@ static int acpi_tie_acpi_dev(struct acpi
 	return 0;
 }
 
-int acpi_device_add(struct acpi_device *device,
-		    void (*release)(struct device *))
+int __acpi_device_add(struct acpi_device *device,
+		      void (*release)(struct device *))
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
 	int result;
 
-	result = acpi_tie_acpi_dev(device);
-	if (result)
-		return result;
-
 	/*
 	 * Linkage
 	 * -------
@@ -755,6 +746,17 @@ err_unlock:
 	return result;
 }
 
+int acpi_device_add(struct acpi_device *adev, void (*release)(struct device *))
+{
+	int ret;
+
+	ret = acpi_tie_acpi_dev(adev);
+	if (ret)
+		return ret;
+
+	return __acpi_device_add(adev, release);
+}
+
 /* --------------------------------------------------------------------------
                                  Device Enumeration
    -------------------------------------------------------------------------- */
@@ -1681,14 +1683,10 @@ static void acpi_scan_dep_init(struct ac
 {
 	struct acpi_dep_data *dep;
 
-	mutex_lock(&acpi_dep_list_lock);
-
 	list_for_each_entry(dep, &acpi_dep_list, node) {
 		if (dep->consumer == adev->handle)
 			adev->dep_unmet++;
 	}
-
-	mutex_unlock(&acpi_dep_list_lock);
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
@@ -1707,6 +1705,7 @@ static int acpi_add_single_object(struct
 				  acpi_handle handle, int type, bool dep_init)
 {
 	struct acpi_device *device;
+	bool release_dep_lock = false;
 	int result;
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
@@ -1720,16 +1719,31 @@ static int acpi_add_single_object(struct
 	 * this must be done before the get power-/wakeup_dev-flags calls.
 	 */
 	if (type == ACPI_BUS_TYPE_DEVICE || type == ACPI_BUS_TYPE_PROCESSOR) {
-		if (dep_init)
+		if (dep_init) {
+			mutex_lock(&acpi_dep_list_lock);
+			/*
+			 * Hold the lock until the acpi_tie_acpi_dev() call
+			 * below to prevent concurrent acpi_scan_clear_dep()
+			 * from deleting a dependency list entry without
+			 * updating dep_unmet for the device.
+			 */
+			release_dep_lock = true;
 			acpi_scan_dep_init(device);
-
+		}
 		acpi_scan_init_status(device);
 	}
 
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
 
-	result = acpi_device_add(device, acpi_device_release);
+	result = acpi_tie_acpi_dev(device);
+
+	if (release_dep_lock)
+		mutex_unlock(&acpi_dep_list_lock);
+
+	if (!result)
+		result = __acpi_device_add(device, acpi_device_release);
+
 	if (result) {
 		acpi_device_release(&device->dev);
 		return result;




  parent reply	other threads:[~2021-06-16 18:00 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-16 14:21 [PATCH 0/5] ACPI: scan: Fixes and cleanups related to dependencies list handling Rafael J. Wysocki
2021-06-16 14:21 ` [PATCH 1/5] ACPI: scan: Rearrange acpi_dev_get_first_consumer_dev_cb() Rafael J. Wysocki
2021-06-16 14:36   ` Hans de Goede
2021-06-16 14:22 ` [PATCH 2/5] ACPI: scan: Make acpi_walk_dep_device_list() Rafael J. Wysocki
2021-06-16 14:41   ` Hans de Goede
2021-06-16 15:11     ` Rafael J. Wysocki
2021-06-16 15:25       ` Rafael J. Wysocki
2021-06-16 15:28       ` Hans de Goede
2021-06-16 14:23 ` [PATCH 3/5] ACPI: scan: Fix device object rescan in acpi_scan_clear_dep() Rafael J. Wysocki
2021-06-16 14:48   ` Hans de Goede
2021-06-16 15:12     ` Rafael J. Wysocki
2021-06-16 14:24 ` [PATCH 4/5] ACPI: scan: Reorganize acpi_device_add() Rafael J. Wysocki
2021-06-16 14:49   ` Hans de Goede
2021-06-16 14:25 ` [PATCH 5/5] ACPI: scan: Fix race related to dropping dependencies Rafael J. Wysocki
2021-06-16 14:55   ` Hans de Goede
2021-06-16 15:19     ` Rafael J. Wysocki
2021-06-16 18:00   ` Rafael J. Wysocki [this message]
2021-06-16 18:04     ` [PATCH v2 " Hans de Goede
2021-06-17  0:25   ` [PATCH " kernel test robot
2021-06-17  0:25     ` kernel test robot
2021-06-17  0:41   ` kernel test robot
2021-06-17  0:41     ` kernel test robot
2021-06-17  0:42   ` [RFC PATCH] ACPI: scan: __acpi_device_add() can be static kernel test robot
2021-06-17  0:42     ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=11783109.O9o76ZdvQC@kreacher \
    --to=rjw@rjwysocki.net \
    --cc=hdegoede@redhat.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.