[PATCH v2 0/4] ACPI: NFIT: core: Fix multiple issues related to concurrency and cleanup

[PATCH v2 0/4] ACPI: NFIT: core: Fix multiple issues related to concurrency and cleanup

[Rafael J. Wysocki]

Hi All,

This series is a replacement for

https://lore.kernel.org/linux-acpi/6000262.DvuYhMxLoT@rafael.j.wysocki/

and it addresses some additional issues discovered during the review
of the patch above.  Some of it is based on the sashiko.dev feedback:

https://sashiko.dev/#/patchset/6000262.DvuYhMxLoT%40rafael.j.wysocki

Patch [1/4] adds a NULL pointer check to prevent a possible NULL pointer
dereference from occurring.

Patch [2/4] fixes issues related to acpi_nfit_init() failures.

Patch [3/4] is a preparatory cleanup before the next patch.

Patch [4/4] addresses a possible deadlock during driver removal or
failing probe and missing NVDIMM device notifications from platform
firmware.

Thanks!

[PATCH v2 1/4] ACPI: NFIT: core: Fix possible NULL pointer dereference

[Rafael J. Wysocki]

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

After commit 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before
getting NFIT table"), acpi_nfit_probe() installs an ACPI notify handler
for the NFIT device before checking the presence of the NFIT table.  If
that table is not there, 0 is returned without allocating the acpi_desc
object and setting the driver data pointer of the NFIT device.  If the
platform firmware triggers an NFIT_NOTIFY_UC_MEMORY_ERROR notification
on the NFIT device at that point, acpi_nfit_uc_error_notify() will
dereference a NULL pointer.

Prevent that from occurring by adding an acpi_desc check against NULL
to acpi_nfit_uc_error_notify().

Fixes: 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before getting NFIT table")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: All applicable <stable@vger.kernel.org>
---
 drivers/acpi/nfit/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 5cab62f618c8..8024cd3cad14 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -3442,6 +3442,9 @@ static void acpi_nfit_uc_error_notify(struct device *dev, acpi_handle handle)
 {
 	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
 
+	if (!acpi_desc)
+		return;
+
 	if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)
 		acpi_nfit_ars_rescan(acpi_desc, ARS_REQ_LONG);
 	else
-- 
2.51.0

Re: [PATCH v2 1/4] ACPI: NFIT: core: Fix possible NULL pointer dereference

[Dave Jiang]

On 6/3/26 10:56 AM, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> After commit 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before
> getting NFIT table"), acpi_nfit_probe() installs an ACPI notify handler
> for the NFIT device before checking the presence of the NFIT table.  If
> that table is not there, 0 is returned without allocating the acpi_desc
> object and setting the driver data pointer of the NFIT device.  If the
> platform firmware triggers an NFIT_NOTIFY_UC_MEMORY_ERROR notification
> on the NFIT device at that point, acpi_nfit_uc_error_notify() will
> dereference a NULL pointer.
> 
> Prevent that from occurring by adding an acpi_desc check against NULL
> to acpi_nfit_uc_error_notify().
> 
> Fixes: 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before getting NFIT table")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: All applicable <stable@vger.kernel.org>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>  drivers/acpi/nfit/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 5cab62f618c8..8024cd3cad14 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3442,6 +3442,9 @@ static void acpi_nfit_uc_error_notify(struct device *dev, acpi_handle handle)
>  {
>  	struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(dev);
>  
> +	if (!acpi_desc)
> +		return;
> +
>  	if (acpi_desc->scrub_mode == HW_ERROR_SCRUB_ON)
>  		acpi_nfit_ars_rescan(acpi_desc, ARS_REQ_LONG);
>  	else

[PATCH v2 2/4] ACPI: NFIT: core: Fix acpi_nfit_init() error cleanup

[Rafael J. Wysocki]

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

If acpi_nfit_init() fails after adding the acpi_desc object to the
acpi_descs list, that object is never removed from that list because
the acpi_nfit_shutdown() devm action is not added for the NFIT device
in that case.  Next, the acpi_nfit_init() failure causes
acpi_nfit_probe() to fail, the acpi_desc object is freed, and a
dangling pointer is left behind in the acpi_descs.  Any subsequent
ACPI Machine Check Exception will trigger nfit_handle_mce() which
iterates over acpi_descs and so a use-after-free will occur.

Moreover, if acpi_nfit_probe() returns 0 after installing a notify
handler for the NFIT device and without allocating the acpi_desc
object and setting the NFIT device's driver data pointer, the
acpi_desc object will be allocated by acpi_nfit_update_notify()
and acpi_nfit_init() will be called to initialize it.  Regardless
of whether or not acpi_nfit_init() fails in that case, the
acpi_nfit_shutdown() devm action is not added for the NFIT device
and acpi_desc is never removed from the acpi_descs list.  If the
acpi_desc object is freed subsequently on driver removal, any
subsequent ACPI MCE will lead to a use-after-free like in the
previous case.

To address the first issue mentioned above, make acpi_nfit_probe()
call acpi_nfit_shutdown() directly on acpi_nfit_init() failures and
to address the other one, add a remove callback to the driver and
make it call acpi_nfit_shutdown().  Also, since it is now possible to
pass NULL to acpi_nfit_shutdown() or the acpi_desc object passed to it
may not have been initialized, add checks against NULL for acpi_desc and
its nvdimm_bus field to that function and make acpi_nfit_unregister()
clear the latter after unregistering the NVDIMM bus.

Fixes: a61fe6f7902e ("nfit, tools/testing/nvdimm: unify common init for acpi_nfit_desc")
Fixes: fbabd829fe76 ("acpi, nfit: fix module unload vs workqueue shutdown race")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: All applicable <stable@vger.kernel.org>
---
 drivers/acpi/nfit/core.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 8024cd3cad14..01c73be0bd00 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -3069,6 +3069,8 @@ static void acpi_nfit_unregister(void *data)
 	struct acpi_nfit_desc *acpi_desc = data;
 
 	nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
+	/* The nvdimm_bus object may have been freed, so clear the pointer. */
+	acpi_desc->nvdimm_bus = NULL;
 }
 
 int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size sz)
@@ -3301,7 +3303,10 @@ static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
 void acpi_nfit_shutdown(void *data)
 {
 	struct acpi_nfit_desc *acpi_desc = data;
-	struct device *bus_dev = to_nvdimm_bus_dev(acpi_desc->nvdimm_bus);
+	struct device *bus_dev;
+
+	if (!acpi_desc || !acpi_desc->nvdimm_bus)
+		return;
 
 	/*
 	 * Destruct under acpi_desc_lock so that nfit_handle_mce does not
@@ -3316,6 +3321,7 @@ void acpi_nfit_shutdown(void *data)
 	mutex_unlock(&acpi_desc->init_mutex);
 	cancel_delayed_work_sync(&acpi_desc->dwork);
 
+	bus_dev = to_nvdimm_bus_dev(acpi_desc->nvdimm_bus);
 	/*
 	 * Bounce the nvdimm bus lock to make sure any in-flight
 	 * acpi_nfit_ars_rescan() submissions have had a chance to
@@ -3388,9 +3394,14 @@ static int acpi_nfit_probe(struct platform_device *pdev)
 				sz - sizeof(struct acpi_table_nfit));
 
 	if (rc)
-		return rc;
+		acpi_nfit_shutdown(acpi_desc);
 
-	return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
+	return rc;
+}
+
+static void acpi_nfit_remove(struct platform_device *pdev)
+{
+	acpi_nfit_shutdown(platform_get_drvdata(pdev));
 }
 
 static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
@@ -3474,6 +3485,7 @@ MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
 
 static struct platform_driver acpi_nfit_driver = {
 	.probe = acpi_nfit_probe,
+	.remove = acpi_nfit_remove,
 	.driver = {
 		.name = "acpi-nfit",
 		.acpi_match_table = acpi_nfit_ids,
-- 
2.51.0

Re: [PATCH v2 2/4] ACPI: NFIT: core: Fix acpi_nfit_init() error cleanup

[Dave Jiang]

On 6/3/26 10:57 AM, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> If acpi_nfit_init() fails after adding the acpi_desc object to the
> acpi_descs list, that object is never removed from that list because
> the acpi_nfit_shutdown() devm action is not added for the NFIT device
> in that case.  Next, the acpi_nfit_init() failure causes
> acpi_nfit_probe() to fail, the acpi_desc object is freed, and a
> dangling pointer is left behind in the acpi_descs.  Any subsequent
> ACPI Machine Check Exception will trigger nfit_handle_mce() which
> iterates over acpi_descs and so a use-after-free will occur.
> 
> Moreover, if acpi_nfit_probe() returns 0 after installing a notify
> handler for the NFIT device and without allocating the acpi_desc
> object and setting the NFIT device's driver data pointer, the
> acpi_desc object will be allocated by acpi_nfit_update_notify()
> and acpi_nfit_init() will be called to initialize it.  Regardless
> of whether or not acpi_nfit_init() fails in that case, the
> acpi_nfit_shutdown() devm action is not added for the NFIT device
> and acpi_desc is never removed from the acpi_descs list.  If the
> acpi_desc object is freed subsequently on driver removal, any
> subsequent ACPI MCE will lead to a use-after-free like in the
> previous case.
> 
> To address the first issue mentioned above, make acpi_nfit_probe()
> call acpi_nfit_shutdown() directly on acpi_nfit_init() failures and
> to address the other one, add a remove callback to the driver and
> make it call acpi_nfit_shutdown().  Also, since it is now possible to
> pass NULL to acpi_nfit_shutdown() or the acpi_desc object passed to it
> may not have been initialized, add checks against NULL for acpi_desc and
> its nvdimm_bus field to that function and make acpi_nfit_unregister()
> clear the latter after unregistering the NVDIMM bus.
> 
> Fixes: a61fe6f7902e ("nfit, tools/testing/nvdimm: unify common init for acpi_nfit_desc")
> Fixes: fbabd829fe76 ("acpi, nfit: fix module unload vs workqueue shutdown race")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: All applicable <stable@vger.kernel.org>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>  drivers/acpi/nfit/core.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 8024cd3cad14..01c73be0bd00 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3069,6 +3069,8 @@ static void acpi_nfit_unregister(void *data)
>  	struct acpi_nfit_desc *acpi_desc = data;
>  
>  	nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
> +	/* The nvdimm_bus object may have been freed, so clear the pointer. */
> +	acpi_desc->nvdimm_bus = NULL;
>  }
>  
>  int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size sz)
> @@ -3301,7 +3303,10 @@ static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>  void acpi_nfit_shutdown(void *data)
>  {
>  	struct acpi_nfit_desc *acpi_desc = data;
> -	struct device *bus_dev = to_nvdimm_bus_dev(acpi_desc->nvdimm_bus);
> +	struct device *bus_dev;
> +
> +	if (!acpi_desc || !acpi_desc->nvdimm_bus)
> +		return;
>  
>  	/*
>  	 * Destruct under acpi_desc_lock so that nfit_handle_mce does not
> @@ -3316,6 +3321,7 @@ void acpi_nfit_shutdown(void *data)
>  	mutex_unlock(&acpi_desc->init_mutex);
>  	cancel_delayed_work_sync(&acpi_desc->dwork);
>  
> +	bus_dev = to_nvdimm_bus_dev(acpi_desc->nvdimm_bus);
>  	/*
>  	 * Bounce the nvdimm bus lock to make sure any in-flight
>  	 * acpi_nfit_ars_rescan() submissions have had a chance to
> @@ -3388,9 +3394,14 @@ static int acpi_nfit_probe(struct platform_device *pdev)
>  				sz - sizeof(struct acpi_table_nfit));
>  
>  	if (rc)
> -		return rc;
> +		acpi_nfit_shutdown(acpi_desc);
>  
> -	return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> +	return rc;
> +}
> +
> +static void acpi_nfit_remove(struct platform_device *pdev)
> +{
> +	acpi_nfit_shutdown(platform_get_drvdata(pdev));
>  }
>  
>  static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
> @@ -3474,6 +3485,7 @@ MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>  
>  static struct platform_driver acpi_nfit_driver = {
>  	.probe = acpi_nfit_probe,
> +	.remove = acpi_nfit_remove,
>  	.driver = {
>  		.name = "acpi-nfit",
>  		.acpi_match_table = acpi_nfit_ids,

[PATCH v2 3/4] ACPI: NFIT: core: Eliminate redundant local variable

[Rafael J. Wysocki]

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

Eliminate local variable acpi_desc from __acpi_nvdimm_notify() because it
is redundant (its value is only checked against NULL once and the value
assigned to it may be checked directly instead) and update the subsequent
comment to reflect the code change.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/nfit/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 01c73be0bd00..aaa84ae7a20e 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1680,7 +1680,6 @@ static struct nvdimm *acpi_nfit_dimm_by_handle(struct acpi_nfit_desc *acpi_desc,
 void __acpi_nvdimm_notify(struct device *dev, u32 event)
 {
 	struct nfit_mem *nfit_mem;
-	struct acpi_nfit_desc *acpi_desc;
 
 	dev_dbg(dev->parent, "%s: event: %d\n", dev_name(dev),
 			event);
@@ -1691,12 +1690,11 @@ void __acpi_nvdimm_notify(struct device *dev, u32 event)
 		return;
 	}
 
-	acpi_desc = dev_get_drvdata(dev->parent);
-	if (!acpi_desc)
+	if (!dev_get_drvdata(dev->parent))
 		return;
 
 	/*
-	 * If we successfully retrieved acpi_desc, then we know nfit_mem data
+	 * If the parent's driver data pointer is not NULL, then nfit_mem data
 	 * is still valid.
 	 */
 	nfit_mem = dev_get_drvdata(dev);
-- 
2.51.0

Re: [PATCH v2 3/4] ACPI: NFIT: core: Eliminate redundant local variable

[Dave Jiang]

On 6/3/26 10:57 AM, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> Eliminate local variable acpi_desc from __acpi_nvdimm_notify() because it
> is redundant (its value is only checked against NULL once and the value
> assigned to it may be checked directly instead) and update the subsequent
> comment to reflect the code change.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>  drivers/acpi/nfit/core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 01c73be0bd00..aaa84ae7a20e 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1680,7 +1680,6 @@ static struct nvdimm *acpi_nfit_dimm_by_handle(struct acpi_nfit_desc *acpi_desc,
>  void __acpi_nvdimm_notify(struct device *dev, u32 event)
>  {
>  	struct nfit_mem *nfit_mem;
> -	struct acpi_nfit_desc *acpi_desc;
>  
>  	dev_dbg(dev->parent, "%s: event: %d\n", dev_name(dev),
>  			event);
> @@ -1691,12 +1690,11 @@ void __acpi_nvdimm_notify(struct device *dev, u32 event)
>  		return;
>  	}
>  
> -	acpi_desc = dev_get_drvdata(dev->parent);
> -	if (!acpi_desc)
> +	if (!dev_get_drvdata(dev->parent))
>  		return;
>  
>  	/*
> -	 * If we successfully retrieved acpi_desc, then we know nfit_mem data
> +	 * If the parent's driver data pointer is not NULL, then nfit_mem data
>  	 * is still valid.
>  	 */
>  	nfit_mem = dev_get_drvdata(dev);

[PATCH v2 4/4] ACPI: NFIT: core: Fix possible deadlock and missing notifications

[Rafael J. Wysocki]

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

After commit 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before
getting NFIT table"), ACPI NFIT driver removal may deadlock if an ACPI
notify on the NFIT device is triggered concurrently.  A similar deadlock
may occur if an ACPI notify on the NFIT device is triggered during a
failing driver probe.

The deadlock is possible because acpi_dev_remove_notify_handler() calls
acpi_os_wait_events_complete() after removing the notify handler and the
driver core invokes it under the NFIT platform device lock which is also
acquired by acpi_nfit_notify().  Thus acpi_os_wait_events_complete() may
be waiting for acpi_nfit_notify() to complete, but the latter may not be
able to acquire the device lock which is being held by the driver core
while the former is being executed.

Moreover, after commit 03667e146f81 ("ACPI: NFIT: core: Convert the
driver to a platform one"), there are no sysfs notifications regarding
NVDIMM devices because __acpi_nvdimm_notify() always bails out after
checking the driver data pointer of the device's parent.  That parent
is the ACPI companion of the platform device used for driver binding,
so its driver data pointer is always NULL after the commit in question
which was overlooked by it.

A remedy for the deadlock is to use a special separate lock for ACPI
notify synchronization with driver probe and removal instead of the
device lock of the NFIT device, while a remedy for the second issue
is to populate the driver data pointer of the NFIT device's ACPI
companion when the driver is ready to operate, so do both these things.
However, since the new lock is not held across the entire teardown and
acpi_nfit_notify() should do nothing when teardown is in progress, make
it check the driver data pointer of the NFIT device's ACPI companion, in
analogy with the existing check in __acpi_nvdimm_notify(), and bail out
if that pointer is NULL.

Fixes: 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before getting NFIT table")
Fixes: 03667e146f81 ("ACPI: NFIT: core: Convert the driver to a platform one")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: All applicable <stable@vger.kernel.org>
---
 drivers/acpi/nfit/core.c | 63 ++++++++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index aaa84ae7a20e..cb771d9cadb2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -56,6 +56,8 @@ MODULE_PARM_DESC(force_labels, "Opt-in to labels despite missing methods");
 LIST_HEAD(acpi_descs);
 DEFINE_MUTEX(acpi_desc_lock);
 
+DEFINE_MUTEX(acpi_notify_lock);
+
 static struct workqueue_struct *nfit_wq;
 
 struct nfit_table_prev {
@@ -1708,9 +1710,15 @@ static void acpi_nvdimm_notify(acpi_handle handle, u32 event, void *data)
 	struct acpi_device *adev = data;
 	struct device *dev = &adev->dev;
 
-	device_lock(dev->parent);
+	/*
+	 * Locking is needed here for synchronization with driver probe and
+	 * removal and the parent's driver data pointer is NULL when teardown
+	 * is in progress (while the parent here is expected to be the ACPI
+	 * companion of the platform device used for driver binding).
+	 */
+	guard(mutex)(&acpi_notify_lock);
+
 	__acpi_nvdimm_notify(dev, event);
-	device_unlock(dev->parent);
 }
 
 static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)
@@ -3156,11 +3164,10 @@ EXPORT_SYMBOL_GPL(acpi_nfit_init);
 static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
 {
 	struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
-	struct device *dev = acpi_desc->dev;
 
-	/* Bounce the device lock to flush acpi_nfit_add / acpi_nfit_notify */
-	device_lock(dev);
-	device_unlock(dev);
+	/* Bounce the notify lock to flush acpi_nfit_probe / acpi_nfit_notify */
+	mutex_lock(&acpi_notify_lock);
+	mutex_unlock(&acpi_notify_lock);
 
 	/* Bounce the init_mutex to complete initial registration */
 	mutex_lock(&acpi_desc->init_mutex);
@@ -3292,10 +3299,17 @@ static void acpi_nfit_put_table(void *table)
 static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct device *dev = data;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 
-	device_lock(dev);
-	__acpi_nfit_notify(dev, handle, event);
-	device_unlock(dev);
+	/*
+	 * Locking is needed here for synchronization with driver probe and
+	 * removal and the ACPI companion's driver data pointer is NULL when
+	 * teardown is in progress.
+	 */
+	guard(mutex)(&acpi_notify_lock);
+
+	if (dev_get_drvdata(&adev->dev))
+		__acpi_nfit_notify(dev, handle, event);
 }
 
 void acpi_nfit_shutdown(void *data)
@@ -3337,11 +3351,18 @@ static int acpi_nfit_probe(struct platform_device *pdev)
 	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
 	struct acpi_nfit_desc *acpi_desc;
 	struct device *dev = &pdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 	struct acpi_table_header *tbl;
 	acpi_status status = AE_OK;
 	acpi_size sz;
 	int rc = 0;
 
+	/*
+	 * Prevent acpi_nfit_notify() from progressing until the probe is
+	 * complete in case there is a concurrent event to process.
+	 */
+	guard(mutex)(&acpi_notify_lock);
+
 	rc = devm_acpi_install_notify_handler(dev, ACPI_DEVICE_NOTIFY,
 					      acpi_nfit_notify, dev);
 	if (rc)
@@ -3357,6 +3378,11 @@ static int acpi_nfit_probe(struct platform_device *pdev)
 		 * data in the format of a series of NFIT Structures.
 		 */
 		dev_dbg(dev, "failed to find NFIT at startup\n");
+		/*
+		 * Let acpi_nfit_update_notify() run in case it will need to
+		 * allocate the acpi_desc object.
+		 */
+		dev_set_drvdata(&adev->dev, dev);
 		return 0;
 	}
 
@@ -3374,7 +3400,7 @@ static int acpi_nfit_probe(struct platform_device *pdev)
 	acpi_desc->acpi_header = *tbl;
 
 	/* Evaluate _FIT and override with that if present */
-	status = acpi_evaluate_object(ACPI_HANDLE(dev), "_FIT", NULL, &buf);
+	status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf);
 	if (ACPI_SUCCESS(status) && buf.length > 0) {
 		union acpi_object *obj = buf.pointer;
 
@@ -3391,14 +3417,27 @@ static int acpi_nfit_probe(struct platform_device *pdev)
 				+ sizeof(struct acpi_table_nfit),
 				sz - sizeof(struct acpi_table_nfit));
 
-	if (rc)
+	if (rc) {
 		acpi_nfit_shutdown(acpi_desc);
+		return rc;
+	}
 
-	return rc;
+	/*
+	 * Let notify handlers operate (the actual value of the ACPI companion's
+	 * driver data pointer does not matter here so long as it is not NULL).
+	 */
+	dev_set_drvdata(&adev->dev, dev);
+	return 0;
 }
 
 static void acpi_nfit_remove(struct platform_device *pdev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+
+	guard(mutex)(&acpi_notify_lock);
+
+	/* Make notify handlers bail out early going forward. */
+	dev_set_drvdata(&adev->dev, NULL);
 	acpi_nfit_shutdown(platform_get_drvdata(pdev));
 }
 
-- 
2.51.0

Re: [PATCH v2 4/4] ACPI: NFIT: core: Fix possible deadlock and missing notifications

[Dave Jiang]

On 6/3/26 10:58 AM, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> After commit 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before
> getting NFIT table"), ACPI NFIT driver removal may deadlock if an ACPI
> notify on the NFIT device is triggered concurrently.  A similar deadlock
> may occur if an ACPI notify on the NFIT device is triggered during a
> failing driver probe.
> 
> The deadlock is possible because acpi_dev_remove_notify_handler() calls
> acpi_os_wait_events_complete() after removing the notify handler and the
> driver core invokes it under the NFIT platform device lock which is also
> acquired by acpi_nfit_notify().  Thus acpi_os_wait_events_complete() may
> be waiting for acpi_nfit_notify() to complete, but the latter may not be
> able to acquire the device lock which is being held by the driver core
> while the former is being executed.
> 
> Moreover, after commit 03667e146f81 ("ACPI: NFIT: core: Convert the
> driver to a platform one"), there are no sysfs notifications regarding
> NVDIMM devices because __acpi_nvdimm_notify() always bails out after
> checking the driver data pointer of the device's parent.  That parent
> is the ACPI companion of the platform device used for driver binding,
> so its driver data pointer is always NULL after the commit in question
> which was overlooked by it.
> 
> A remedy for the deadlock is to use a special separate lock for ACPI
> notify synchronization with driver probe and removal instead of the
> device lock of the NFIT device, while a remedy for the second issue
> is to populate the driver data pointer of the NFIT device's ACPI
> companion when the driver is ready to operate, so do both these things.
> However, since the new lock is not held across the entire teardown and
> acpi_nfit_notify() should do nothing when teardown is in progress, make
> it check the driver data pointer of the NFIT device's ACPI companion, in
> analogy with the existing check in __acpi_nvdimm_notify(), and bail out
> if that pointer is NULL.
> 
> Fixes: 9b311b7313d6 ("ACPI: NFIT: Install Notify() handler before getting NFIT table")
> Fixes: 03667e146f81 ("ACPI: NFIT: core: Convert the driver to a platform one")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: All applicable <stable@vger.kernel.org>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>  drivers/acpi/nfit/core.c | 63 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index aaa84ae7a20e..cb771d9cadb2 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -56,6 +56,8 @@ MODULE_PARM_DESC(force_labels, "Opt-in to labels despite missing methods");
>  LIST_HEAD(acpi_descs);
>  DEFINE_MUTEX(acpi_desc_lock);
>  
> +DEFINE_MUTEX(acpi_notify_lock);
> +
>  static struct workqueue_struct *nfit_wq;
>  
>  struct nfit_table_prev {
> @@ -1708,9 +1710,15 @@ static void acpi_nvdimm_notify(acpi_handle handle, u32 event, void *data)
>  	struct acpi_device *adev = data;
>  	struct device *dev = &adev->dev;
>  
> -	device_lock(dev->parent);
> +	/*
> +	 * Locking is needed here for synchronization with driver probe and
> +	 * removal and the parent's driver data pointer is NULL when teardown
> +	 * is in progress (while the parent here is expected to be the ACPI
> +	 * companion of the platform device used for driver binding).
> +	 */
> +	guard(mutex)(&acpi_notify_lock);
> +
>  	__acpi_nvdimm_notify(dev, event);
> -	device_unlock(dev->parent);
>  }
>  
>  static bool acpi_nvdimm_has_method(struct acpi_device *adev, char *method)
> @@ -3156,11 +3164,10 @@ EXPORT_SYMBOL_GPL(acpi_nfit_init);
>  static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
>  {
>  	struct acpi_nfit_desc *acpi_desc = to_acpi_desc(nd_desc);
> -	struct device *dev = acpi_desc->dev;
>  
> -	/* Bounce the device lock to flush acpi_nfit_add / acpi_nfit_notify */
> -	device_lock(dev);
> -	device_unlock(dev);
> +	/* Bounce the notify lock to flush acpi_nfit_probe / acpi_nfit_notify */
> +	mutex_lock(&acpi_notify_lock);
> +	mutex_unlock(&acpi_notify_lock);
>  
>  	/* Bounce the init_mutex to complete initial registration */
>  	mutex_lock(&acpi_desc->init_mutex);
> @@ -3292,10 +3299,17 @@ static void acpi_nfit_put_table(void *table)
>  static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>  {
>  	struct device *dev = data;
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>  
> -	device_lock(dev);
> -	__acpi_nfit_notify(dev, handle, event);
> -	device_unlock(dev);
> +	/*
> +	 * Locking is needed here for synchronization with driver probe and
> +	 * removal and the ACPI companion's driver data pointer is NULL when
> +	 * teardown is in progress.
> +	 */
> +	guard(mutex)(&acpi_notify_lock);
> +
> +	if (dev_get_drvdata(&adev->dev))
> +		__acpi_nfit_notify(dev, handle, event);
>  }
>  
>  void acpi_nfit_shutdown(void *data)
> @@ -3337,11 +3351,18 @@ static int acpi_nfit_probe(struct platform_device *pdev)
>  	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>  	struct acpi_nfit_desc *acpi_desc;
>  	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
>  	struct acpi_table_header *tbl;
>  	acpi_status status = AE_OK;
>  	acpi_size sz;
>  	int rc = 0;
>  
> +	/*
> +	 * Prevent acpi_nfit_notify() from progressing until the probe is
> +	 * complete in case there is a concurrent event to process.
> +	 */
> +	guard(mutex)(&acpi_notify_lock);
> +
>  	rc = devm_acpi_install_notify_handler(dev, ACPI_DEVICE_NOTIFY,
>  					      acpi_nfit_notify, dev);
>  	if (rc)
> @@ -3357,6 +3378,11 @@ static int acpi_nfit_probe(struct platform_device *pdev)
>  		 * data in the format of a series of NFIT Structures.
>  		 */
>  		dev_dbg(dev, "failed to find NFIT at startup\n");
> +		/*
> +		 * Let acpi_nfit_update_notify() run in case it will need to
> +		 * allocate the acpi_desc object.
> +		 */
> +		dev_set_drvdata(&adev->dev, dev);
>  		return 0;
>  	}
>  
> @@ -3374,7 +3400,7 @@ static int acpi_nfit_probe(struct platform_device *pdev)
>  	acpi_desc->acpi_header = *tbl;
>  
>  	/* Evaluate _FIT and override with that if present */
> -	status = acpi_evaluate_object(ACPI_HANDLE(dev), "_FIT", NULL, &buf);
> +	status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf);
>  	if (ACPI_SUCCESS(status) && buf.length > 0) {
>  		union acpi_object *obj = buf.pointer;
>  
> @@ -3391,14 +3417,27 @@ static int acpi_nfit_probe(struct platform_device *pdev)
>  				+ sizeof(struct acpi_table_nfit),
>  				sz - sizeof(struct acpi_table_nfit));
>  
> -	if (rc)
> +	if (rc) {
>  		acpi_nfit_shutdown(acpi_desc);
> +		return rc;
> +	}
>  
> -	return rc;
> +	/*
> +	 * Let notify handlers operate (the actual value of the ACPI companion's
> +	 * driver data pointer does not matter here so long as it is not NULL).
> +	 */
> +	dev_set_drvdata(&adev->dev, dev);
> +	return 0;
>  }
>  
>  static void acpi_nfit_remove(struct platform_device *pdev)
>  {
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +
> +	guard(mutex)(&acpi_notify_lock);
> +
> +	/* Make notify handlers bail out early going forward. */
> +	dev_set_drvdata(&adev->dev, NULL);
>  	acpi_nfit_shutdown(platform_get_drvdata(pdev));
>  }
>