diff mbox series

[v1] PM: runtime: PCI: Drain runtime-idle callbacks before driver removal

Message ID 5761426.DvuYhMxLoT@kreacher (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series [v1] PM: runtime: PCI: Drain runtime-idle callbacks before driver removal | expand

Commit Message

Rafael J. Wysocki March 5, 2024, 10:45 a.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

A race condition between the .runtime_idle() callback and the .remove()
callback in the rtsx_pcr PCI driver leads to a kernel crash due to an
unhandled page fault [1].

The problem is that rtsx_pci_runtime_idle() is not expected to be
running after pm_runtime_get_sync() has been called, but the latter
doesn't really guarantee that.  It only guarantees that the suspend
and resume callbacks will not be running when it returns.

However, if a .runtime_idle() callback is already running when
pm_runtime_get_sync() is called, the latter will notice that the
runtime PM status of the device is RPM_ACTIVE and it will return right
away without waiting for the former to complete.  In fact, it cannot
wait for .runtime_idle() to complete because it may be called from that
callback (it arguably does not make much sense to do that, but it is not
strictly prohibited).

Thus in general, whoever is providing a .runtime_idle() callback, they
need to protect it from running in parallel with whatever code runs
after pm_runtime_get_sync().  [Note that .runtime_idle() will not start
after pm_runtime_get_sync() has returned, but it may continue running
then if it has started earlier already.]

One way to address that race condition is to call pm_runtime_barrier()
after pm_runtime_get_sync() (not before it, because a nonzero value of
the runtime PM usage counter is necessary to prevent runtime PM
callbacks from being invoked) to wait for the runtime-idle callback to
complete should it be running at that point.  A suitable place for
doing that is in pci_device_remove() which calls pm_runtime_get_sync()
before removing the driver, so it may as well call pm_runtime_barrier()
subsequently, which will prevent the race in question from occurring,
not just in the rtsx_pcr driver, but in any PCI drivers providing
runtime-idle callbacks.

Link: https://lore.kernel.org/lkml/20240229062201.49500-1-kai.heng.feng@canonical.com/ # [1]
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Tested-by: Ricky Wu <ricky_wu@realtek.com>
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |    7 +++++++
 1 file changed, 7 insertions(+)

Comments

Kai-Heng Feng March 5, 2024, 1:36 p.m. UTC | #1 
On Tue, Mar 5, 2024 at 6:45 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> A race condition between the .runtime_idle() callback and the .remove()
> callback in the rtsx_pcr PCI driver leads to a kernel crash due to an
> unhandled page fault [1].
>
> The problem is that rtsx_pci_runtime_idle() is not expected to be
> running after pm_runtime_get_sync() has been called, but the latter
> doesn't really guarantee that.  It only guarantees that the suspend
> and resume callbacks will not be running when it returns.
>
> However, if a .runtime_idle() callback is already running when
> pm_runtime_get_sync() is called, the latter will notice that the
> runtime PM status of the device is RPM_ACTIVE and it will return right
> away without waiting for the former to complete.  In fact, it cannot
> wait for .runtime_idle() to complete because it may be called from that
> callback (it arguably does not make much sense to do that, but it is not
> strictly prohibited).
>
> Thus in general, whoever is providing a .runtime_idle() callback, they
> need to protect it from running in parallel with whatever code runs
> after pm_runtime_get_sync().  [Note that .runtime_idle() will not start
> after pm_runtime_get_sync() has returned, but it may continue running
> then if it has started earlier already.]
>
> One way to address that race condition is to call pm_runtime_barrier()
> after pm_runtime_get_sync() (not before it, because a nonzero value of
> the runtime PM usage counter is necessary to prevent runtime PM
> callbacks from being invoked) to wait for the runtime-idle callback to
> complete should it be running at that point.  A suitable place for
> doing that is in pci_device_remove() which calls pm_runtime_get_sync()
> before removing the driver, so it may as well call pm_runtime_barrier()
> subsequently, which will prevent the race in question from occurring,
> not just in the rtsx_pcr driver, but in any PCI drivers providing
> runtime-idle callbacks.
>
> Link: https://lore.kernel.org/lkml/20240229062201.49500-1-kai.heng.feng@canonical.com/ # [1]
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Ricky Wu <ricky_wu@realtek.com>
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks for the debugging and patch.
Acked-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

> ---
>  drivers/pci/pci-driver.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -473,6 +473,13 @@ static void pci_device_remove(struct dev
>
>         if (drv->remove) {
>                 pm_runtime_get_sync(dev);
> +               /*
> +                * If the driver provides a .runtime_idle() callback and it has
> +                * started to run already, it may continue to run in parallel
> +                * with the code below, so wait until all of the runtime PM
> +                * activity has completed.
> +                */
> +               pm_runtime_barrier(dev);
>                 drv->remove(pci_dev);
>                 pm_runtime_put_noidle(dev);
>         }
>
>
>
Bjorn Helgaas March 5, 2024, 6:14 p.m. UTC | #2 
On Tue, Mar 05, 2024 at 11:45:38AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> A race condition between the .runtime_idle() callback and the .remove()
> callback in the rtsx_pcr PCI driver leads to a kernel crash due to an
> unhandled page fault [1].
> 
> The problem is that rtsx_pci_runtime_idle() is not expected to be
> running after pm_runtime_get_sync() has been called, but the latter
> doesn't really guarantee that.  It only guarantees that the suspend
> and resume callbacks will not be running when it returns.
> 
> However, if a .runtime_idle() callback is already running when
> pm_runtime_get_sync() is called, the latter will notice that the
> runtime PM status of the device is RPM_ACTIVE and it will return right
> away without waiting for the former to complete.  In fact, it cannot
> wait for .runtime_idle() to complete because it may be called from that
> callback (it arguably does not make much sense to do that, but it is not
> strictly prohibited).
> 
> Thus in general, whoever is providing a .runtime_idle() callback, they
> need to protect it from running in parallel with whatever code runs
> after pm_runtime_get_sync().  [Note that .runtime_idle() will not start
> after pm_runtime_get_sync() has returned, but it may continue running
> then if it has started earlier already.]
> 
> One way to address that race condition is to call pm_runtime_barrier()
> after pm_runtime_get_sync() (not before it, because a nonzero value of
> the runtime PM usage counter is necessary to prevent runtime PM
> callbacks from being invoked) to wait for the runtime-idle callback to
> complete should it be running at that point.  A suitable place for
> doing that is in pci_device_remove() which calls pm_runtime_get_sync()
> before removing the driver, so it may as well call pm_runtime_barrier()
> subsequently, which will prevent the race in question from occurring,
> not just in the rtsx_pcr driver, but in any PCI drivers providing
> runtime-idle callbacks.
> 
> Link: https://lore.kernel.org/lkml/20240229062201.49500-1-kai.heng.feng@canonical.com/ # [1]
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Tested-by: Ricky Wu <ricky_wu@realtek.com>
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Applied with Kai-Feng's ack to pci/pm for v6.9, thank you very much!

> ---
>  drivers/pci/pci-driver.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -473,6 +473,13 @@ static void pci_device_remove(struct dev
>  
>  	if (drv->remove) {
>  		pm_runtime_get_sync(dev);
> +		/*
> +		 * If the driver provides a .runtime_idle() callback and it has
> +		 * started to run already, it may continue to run in parallel
> +		 * with the code below, so wait until all of the runtime PM
> +		 * activity has completed.
> +		 */
> +		pm_runtime_barrier(dev);
>  		drv->remove(pci_dev);
>  		pm_runtime_put_noidle(dev);
>  	}
> 
> 
>
diff mbox series

Patch

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -473,6 +473,13 @@  static void pci_device_remove(struct dev
 
 	if (drv->remove) {
 		pm_runtime_get_sync(dev);
+		/*
+		 * If the driver provides a .runtime_idle() callback and it has
+		 * started to run already, it may continue to run in parallel
+		 * with the code below, so wait until all of the runtime PM
+		 * activity has completed.
+		 */
+		pm_runtime_barrier(dev);
 		drv->remove(pci_dev);
 		pm_runtime_put_noidle(dev);
 	}