[PATCH 0/3] remoteproc: Improve removal of remoteproc drivers

[PATCH 0/3] remoteproc: Improve removal of remoteproc drivers

[Bjorn Andersson]

Stopping a remoteproc that has entered a crash loop can be problematic,
as shutdown is only allowed while the instance is in its running state.

One effect of this is that a remoteproc which has entered a crash state,
with `recovery_disabled` can not be stopped, without first attempting
the recovery.

This becomes of particular interest if the user rmmod a remoteproc
driver in such state, since the return value of rproc_shutdown() and
rproc_del() are ignored, which means that the remoteproc is left running
while the remoteproc driver will unmap memory and shut down critical
resources under its feet.

Another variant of this issue comes from the lack of synchronization
between rproc_del() and the crash handler worker, which might be
scheduled concurrently with the removal. Mutual exclusion does ensure
that concurrent progress isn't made, but nothing prevent rproc_del()
from returning to the remoteproc driver and have all the resources
freed/unmapped while the worker is still running.

Lastly, another way to trigger this faulty state is to bump the refcount
for the remoteproc, in which case rproc_shutdown() becomes a nop,
again leaving the remoteproc running while the remoteproc driver removes
critical resources.

There's certainly more work needed in these code paths, but this settles
a few obvious issues.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
Bjorn Andersson (3):
      remoteproc: Allow shutdown of crashed processors
      remoteproc: Prevent crash handling to race with rproc_del()
      remoteproc: Force shutdown during device removal

 drivers/remoteproc/remoteproc_core.c  | 104 ++++++++++++++++++++++------------
 drivers/remoteproc/remoteproc_sysfs.c |   1 -
 include/linux/remoteproc.h            |  13 +++--
 3 files changed, 76 insertions(+), 42 deletions(-)
---
base-commit: 290aaf24a551d5a0dce037e3fab30820f9113a10
change-id: 20260723-rproc-rmmod-not-crashing-d92388aa47aa

Best regards,
--  
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

[PATCH 1/3] remoteproc: Allow shutdown of crashed processors

[Bjorn Andersson]

rproc_shutdown() rejects a remoteproc in RPROC_CRASHED state, and
rproc_del() ignores that error. The result of these two decisions is
that a user cannot stop a remoteproc that with recovery disabled that
has entered a crash state, and removal of an associated remoteproc
driver will release resources without first stopping the remoteproc.

Allow rproc_shutdown() to stop crashed processors. Propagate the crash
state to subdevice teardown, to allow subdevices to dismantle things
appropriately.

Assisted-by: OpenCode:GPT-5.5
Fixes: 5e6a0e05270e ("remoteproc: core: Move state checking to remoteproc_core")
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/remoteproc/remoteproc_core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0583d3a56212..3cd4570513c2 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2002,6 +2002,7 @@ EXPORT_SYMBOL(rproc_boot);
 int rproc_shutdown(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
+	bool crashed;
 	int ret;
 
 	ret = mutex_lock_interruptible(&rproc->lock);
@@ -2011,16 +2012,18 @@ int rproc_shutdown(struct rproc *rproc)
 	}
 
 	if (rproc->state != RPROC_RUNNING &&
-	    rproc->state != RPROC_ATTACHED) {
+	    rproc->state != RPROC_ATTACHED &&
+	    rproc->state != RPROC_CRASHED) {
 		ret = -EINVAL;
 		goto out;
 	}
+	crashed = rproc->state == RPROC_CRASHED;
 
 	/* if the remote proc is still needed, bail out */
 	if (!atomic_dec_and_test(&rproc->power))
 		goto out;
 
-	ret = rproc_stop(rproc, false);
+	ret = rproc_stop(rproc, crashed);
 	if (ret) {
 		atomic_inc(&rproc->power);
 		goto out;

-- 
2.53.0

[PATCH 2/3] remoteproc: Prevent crash handling to race with rproc_del()

[Bjorn Andersson]

There's no synchronization between rproc_crash_handler_work() and
rproc_del(), as such it's possible for a driver to be removed while
crash-handler work is scheduled, or even executing - resulting in
use-after-free issues.

To avoid this the scheduled work need to be cancelled and synchronized
against before the removal proceeds.

In order to ensure that this doesn't race with the reporting, and
thereby scheduling new work, a "deleting" flag is introduced. This is
similar to the RPROC_DELETE state that was introduced to ensure that
"start" didn't race with rproc_del(), but the existing mechanism can not
be used as it's valid to call rproc_report_crash() in atomic context -
and the "state" is protected by a mutex.

In the event that work is cancelled the pm_stay_awake() is left
unbalanced and need to be unrolled.

The blocking and cancelling of crash-handler work prior to the actual
rproc_shutdown() call does have the explicit side-effect that crashes
resulting from the shutdown process will not enter the crash-handling
path, and as such will not generate devcoredumps etc. Due to the
existing mutual exclusion between these code paths there's no concrete
reduction in functionality, but further work would be needed to handle
this case.

Assisted-by: OpenCode:GPT-5.5
Fixes: 8afd519c3470 ("remoteproc: add rproc_report_crash function to notify rproc crashes")
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/remoteproc/remoteproc_core.c  | 42 +++++++++++++++++++++++++++--------
 drivers/remoteproc/remoteproc_sysfs.c |  1 -
 include/linux/remoteproc.h            | 13 ++++++-----
 3 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3cd4570513c2..899d2058bc45 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1851,6 +1851,11 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	if (ret)
 		return ret;
 
+	if (READ_ONCE(rproc->deleting)) {
+		ret = -ENODEV;
+		goto unlock_mutex;
+	}
+
 	/* State could have changed before we got the mutex */
 	if (rproc->state != RPROC_CRASHED)
 		goto unlock_mutex;
@@ -1883,6 +1888,11 @@ static void rproc_crash_handler_work(struct work_struct *work)
 
 	mutex_lock(&rproc->lock);
 
+	if (READ_ONCE(rproc->deleting)) {
+		mutex_unlock(&rproc->lock);
+		goto out;
+	}
+
 	if (rproc->state == RPROC_CRASHED) {
 		/* handle only the first crash detected */
 		mutex_unlock(&rproc->lock);
@@ -1938,9 +1948,9 @@ int rproc_boot(struct rproc *rproc)
 		return ret;
 	}
 
-	if (rproc->state == RPROC_DELETED) {
+	if (READ_ONCE(rproc->deleting)) {
 		ret = -ENODEV;
-		dev_err(dev, "can't boot deleted rproc %s\n", rproc->name);
+		dev_err(dev, "can't boot deleting rproc %s\n", rproc->name);
 		goto unlock_mutex;
 	}
 
@@ -2533,8 +2543,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->subdevs);
 	INIT_LIST_HEAD(&rproc->dump_segments);
 
-	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
 	INIT_WORK(&rproc->attach_work, rproc_attach_work);
+	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
+	spin_lock_init(&rproc->crash_handler_lock);
 
 	rproc->state = RPROC_OFFLINE;
 
@@ -2598,16 +2609,21 @@ EXPORT_SYMBOL(rproc_put);
  */
 int rproc_del(struct rproc *rproc)
 {
+	unsigned long flags;
+
 	if (!rproc)
 		return -EINVAL;
 
+	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
+	WRITE_ONCE(rproc->deleting, true);
+	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
+
+	if (cancel_work_sync(&rproc->crash_handler))
+		pm_relax(rproc->dev.parent);
+
 	/* TODO: make sure this works with rproc->power > 1 */
 	rproc_shutdown(rproc);
 
-	mutex_lock(&rproc->lock);
-	rproc->state = RPROC_DELETED;
-	mutex_unlock(&rproc->lock);
-
 	rproc_delete_debug_dir(rproc);
 
 	/* the rproc is downref'ed as soon as it's removed from the klist */
@@ -2719,18 +2735,26 @@ EXPORT_SYMBOL(rproc_get_by_child);
  */
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
 {
+	unsigned long flags;
+
 	if (!rproc) {
 		pr_err("NULL rproc pointer\n");
 		return;
 	}
 
+	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
+	if (READ_ONCE(rproc->deleting)) {
+		spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
+		return;
+	}
+
 	/* Prevent suspend while the remoteproc is being recovered */
 	pm_stay_awake(rproc->dev.parent);
+	queue_work(rproc_recovery_wq, &rproc->crash_handler);
+	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
 
 	dev_err(&rproc->dev, "crash detected in %s: type %s\n",
 		rproc->name, rproc_crash_to_string(type));
-
-	queue_work(rproc_recovery_wq, &rproc->crash_handler);
 }
 EXPORT_SYMBOL(rproc_report_crash);
 
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 138e752c5e4e..925b0cdbe577 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -168,7 +168,6 @@ static const char * const rproc_state_string[] = {
 	[RPROC_SUSPENDED]	= "suspended",
 	[RPROC_RUNNING]		= "running",
 	[RPROC_CRASHED]		= "crashed",
-	[RPROC_DELETED]		= "deleted",
 	[RPROC_ATTACHED]	= "attached",
 	[RPROC_DETACHED]	= "detached",
 	[RPROC_LAST]		= "invalid",
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 17ed75a11e15..ef711a5b1a7f 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -37,6 +37,7 @@
 
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <linux/virtio.h>
 #include <linux/cdev.h>
 #include <linux/completion.h>
@@ -145,7 +146,6 @@ struct rproc_ops {
  *			a message.
  * @RPROC_RUNNING:	device is up and running
  * @RPROC_CRASHED:	device has crashed; need to start recovery
- * @RPROC_DELETED:	device is deleted
  * @RPROC_ATTACHED:	device has been booted by another entity and the core
  *			has attached to it
  * @RPROC_DETACHED:	device has been booted by another entity and waiting
@@ -163,10 +163,9 @@ enum rproc_state {
 	RPROC_SUSPENDED	= 1,
 	RPROC_RUNNING	= 2,
 	RPROC_CRASHED	= 3,
-	RPROC_DELETED	= 4,
-	RPROC_ATTACHED	= 5,
-	RPROC_DETACHED	= 6,
-	RPROC_LAST	= 7,
+	RPROC_ATTACHED	= 4,
+	RPROC_DETACHED	= 5,
+	RPROC_LAST	= 6,
 };
 
 /**
@@ -261,6 +260,8 @@ enum rproc_features {
  * @index: index of this rproc device
  * @attach_work: workqueue for attaching rproc
  * @crash_handler: workqueue for handling a crash
+ * @crash_handler_lock: serializes crash handler queueing and deletion
+ * @deleting: remoteproc deletion has begun
  * @crash_cnt: crash counter
  * @recovery_disabled: flag that state if recovery was disabled
  * @max_notifyid: largest allocated notify id.
@@ -305,6 +306,8 @@ struct rproc {
 	int index;
 	struct work_struct attach_work;
 	struct work_struct crash_handler;
+	spinlock_t crash_handler_lock;
+	bool deleting;
 	unsigned int crash_cnt;
 	bool recovery_disabled;
 	int max_notifyid;

-- 
2.53.0

[PATCH 3/3] remoteproc: Force shutdown during device removal

[Bjorn Andersson]

rproc_del() is subjected to the reference counted shutdown path, as such
a remoteproc with more than one reference will be left running as its
resources are released underneath it.

Refactor the shutdown path such that the reference count value is
ignored when called from rproc_del().

Assisted-by: OpenCode:GPT-5.5
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/remoteproc/remoteproc_core.c | 57 ++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 899d2058bc45..1ed406714849 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1988,28 +1988,7 @@ int rproc_boot(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_boot);
 
-/**
- * rproc_shutdown() - power off the remote processor
- * @rproc: the remote processor
- *
- * Power off a remote processor (previously booted with rproc_boot()).
- *
- * In case @rproc is still being used by an additional user(s), then
- * this function will just decrement the power refcount and exit,
- * without really powering off the device.
- *
- * Every call to rproc_boot() must (eventually) be accompanied by a call
- * to rproc_shutdown(). Calling rproc_shutdown() redundantly is a bug.
- *
- * Notes:
- * - we're not decrementing the rproc's refcount, only the power refcount.
- *   which means that the @rproc handle stays valid even after rproc_shutdown()
- *   returns, and users can still use it with a subsequent rproc_boot(), if
- *   needed.
- *
- * Return: 0 on success, and an appropriate error value otherwise
- */
-int rproc_shutdown(struct rproc *rproc)
+static int __rproc_shutdown(struct rproc *rproc, bool force)
 {
 	struct device *dev = &rproc->dev;
 	bool crashed;
@@ -2029,9 +2008,10 @@ int rproc_shutdown(struct rproc *rproc)
 	}
 	crashed = rproc->state == RPROC_CRASHED;
 
-	/* if the remote proc is still needed, bail out */
-	if (!atomic_dec_and_test(&rproc->power))
+	if (!atomic_dec_and_test(&rproc->power) && !force) {
+		/* The remote processor is still needed by another user. */
 		goto out;
+	}
 
 	ret = rproc_stop(rproc, crashed);
 	if (ret) {
@@ -2055,6 +2035,32 @@ int rproc_shutdown(struct rproc *rproc)
 	mutex_unlock(&rproc->lock);
 	return ret;
 }
+
+/**
+ * rproc_shutdown() - power off the remote processor
+ * @rproc: the remote processor
+ *
+ * Power off a remote processor (previously booted with rproc_boot()).
+ *
+ * In case @rproc is still being used by an additional user(s), then
+ * this function will just decrement the power refcount and exit,
+ * without really powering off the device.
+ *
+ * Every call to rproc_boot() must (eventually) be accompanied by a call
+ * to rproc_shutdown(). Calling rproc_shutdown() redundantly is a bug.
+ *
+ * Notes:
+ * - we're not decrementing the rproc's refcount, only the power refcount.
+ *   which means that the @rproc handle stays valid even after rproc_shutdown()
+ *   returns, and users can still use it with a subsequent rproc_boot(), if
+ *   needed.
+ *
+ * Return: 0 on success, and an appropriate error value otherwise
+ */
+int rproc_shutdown(struct rproc *rproc)
+{
+	return __rproc_shutdown(rproc, false);
+}
 EXPORT_SYMBOL(rproc_shutdown);
 
 /**
@@ -2621,8 +2627,7 @@ int rproc_del(struct rproc *rproc)
 	if (cancel_work_sync(&rproc->crash_handler))
 		pm_relax(rproc->dev.parent);
 
-	/* TODO: make sure this works with rproc->power > 1 */
-	rproc_shutdown(rproc);
+	__rproc_shutdown(rproc, true);
 
 	rproc_delete_debug_dir(rproc);
 

-- 
2.53.0

Re: [PATCH 2/3] remoteproc: Prevent crash handling to race with rproc_del()

[Mukesh Ojha]

On Thu, Jul 23, 2026 at 03:52:28AM +0000, Bjorn Andersson wrote:
> There's no synchronization between rproc_crash_handler_work() and
> rproc_del(), as such it's possible for a driver to be removed while
> crash-handler work is scheduled, or even executing - resulting in
> use-after-free issues.
> 
> To avoid this the scheduled work need to be cancelled and synchronized
> against before the removal proceeds.
> 
> In order to ensure that this doesn't race with the reporting, and
> thereby scheduling new work, a "deleting" flag is introduced. This is
> similar to the RPROC_DELETE state that was introduced to ensure that
> "start" didn't race with rproc_del(), but the existing mechanism can not
> be used as it's valid to call rproc_report_crash() in atomic context -
> and the "state" is protected by a mutex.
> 
> In the event that work is cancelled the pm_stay_awake() is left
> unbalanced and need to be unrolled.
> 
> The blocking and cancelling of crash-handler work prior to the actual
> rproc_shutdown() call does have the explicit side-effect that crashes
> resulting from the shutdown process will not enter the crash-handling
> path, and as such will not generate devcoredumps etc. Due to the
> existing mutual exclusion between these code paths there's no concrete
> reduction in functionality, but further work would be needed to handle
> this case.
> 
> Assisted-by: OpenCode:GPT-5.5
> Fixes: 8afd519c3470 ("remoteproc: add rproc_report_crash function to notify rproc crashes")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  drivers/remoteproc/remoteproc_core.c  | 42 +++++++++++++++++++++++++++--------
>  drivers/remoteproc/remoteproc_sysfs.c |  1 -
>  include/linux/remoteproc.h            | 13 ++++++-----
>  3 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3cd4570513c2..899d2058bc45 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1851,6 +1851,11 @@ int rproc_trigger_recovery(struct rproc *rproc)
>  	if (ret)
>  		return ret;
>  
> +	if (READ_ONCE(rproc->deleting)) {
> +		ret = -ENODEV;
> +		goto unlock_mutex;
> +	}
> +
>  	/* State could have changed before we got the mutex */
>  	if (rproc->state != RPROC_CRASHED)
>  		goto unlock_mutex;
> @@ -1883,6 +1888,11 @@ static void rproc_crash_handler_work(struct work_struct *work)
>  
>  	mutex_lock(&rproc->lock);
>  
> +	if (READ_ONCE(rproc->deleting)) {
> +		mutex_unlock(&rproc->lock);
> +		goto out;
> +	}
> +
>  	if (rproc->state == RPROC_CRASHED) {
>  		/* handle only the first crash detected */
>  		mutex_unlock(&rproc->lock);
> @@ -1938,9 +1948,9 @@ int rproc_boot(struct rproc *rproc)
>  		return ret;
>  	}
>  
> -	if (rproc->state == RPROC_DELETED) {
> +	if (READ_ONCE(rproc->deleting)) {
>  		ret = -ENODEV;
> -		dev_err(dev, "can't boot deleted rproc %s\n", rproc->name);
> +		dev_err(dev, "can't boot deleting rproc %s\n", rproc->name);
>  		goto unlock_mutex;
>  	}
>  
> @@ -2533,8 +2543,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  	INIT_LIST_HEAD(&rproc->subdevs);
>  	INIT_LIST_HEAD(&rproc->dump_segments);
>  
> -	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
>  	INIT_WORK(&rproc->attach_work, rproc_attach_work);
> +	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
> +	spin_lock_init(&rproc->crash_handler_lock);
>  
>  	rproc->state = RPROC_OFFLINE;
>  
> @@ -2598,16 +2609,21 @@ EXPORT_SYMBOL(rproc_put);
>   */
>  int rproc_del(struct rproc *rproc)
>  {
> +	unsigned long flags;
> +
>  	if (!rproc)
>  		return -EINVAL;
>  
> +	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
> +	WRITE_ONCE(rproc->deleting, true);
> +	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> +
> +	if (cancel_work_sync(&rproc->crash_handler))
> +		pm_relax(rproc->dev.parent);
> +
>  	/* TODO: make sure this works with rproc->power > 1 */
>  	rproc_shutdown(rproc);
>  
> -	mutex_lock(&rproc->lock);
> -	rproc->state = RPROC_DELETED;
> -	mutex_unlock(&rproc->lock);
> -
>  	rproc_delete_debug_dir(rproc);
>  
>  	/* the rproc is downref'ed as soon as it's removed from the klist */
> @@ -2719,18 +2735,26 @@ EXPORT_SYMBOL(rproc_get_by_child);
>   */
>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
>  {
> +	unsigned long flags;
> +
>  	if (!rproc) {
>  		pr_err("NULL rproc pointer\n");
>  		return;
>  	}
>  
> +	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
> +	if (READ_ONCE(rproc->deleting)) {
> +		spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> +		return;
> +	}
> +
>  	/* Prevent suspend while the remoteproc is being recovered */
>  	pm_stay_awake(rproc->dev.parent);
> +	queue_work(rproc_recovery_wq, &rproc->crash_handler);

Do we really need this inside the spinlock ?

> +	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
>  
>  	dev_err(&rproc->dev, "crash detected in %s: type %s\n",
>  		rproc->name, rproc_crash_to_string(type));
> -
> -	queue_work(rproc_recovery_wq, &rproc->crash_handler);
>  }
>  EXPORT_SYMBOL(rproc_report_crash);
>  
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 138e752c5e4e..925b0cdbe577 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -168,7 +168,6 @@ static const char * const rproc_state_string[] = {
>  	[RPROC_SUSPENDED]	= "suspended",
>  	[RPROC_RUNNING]		= "running",
>  	[RPROC_CRASHED]		= "crashed",
> -	[RPROC_DELETED]		= "deleted",
>  	[RPROC_ATTACHED]	= "attached",
>  	[RPROC_DETACHED]	= "detached",
>  	[RPROC_LAST]		= "invalid",
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 17ed75a11e15..ef711a5b1a7f 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -37,6 +37,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/spinlock.h>
>  #include <linux/virtio.h>
>  #include <linux/cdev.h>
>  #include <linux/completion.h>
> @@ -145,7 +146,6 @@ struct rproc_ops {
>   *			a message.
>   * @RPROC_RUNNING:	device is up and running
>   * @RPROC_CRASHED:	device has crashed; need to start recovery
> - * @RPROC_DELETED:	device is deleted
>   * @RPROC_ATTACHED:	device has been booted by another entity and the core
>   *			has attached to it
>   * @RPROC_DETACHED:	device has been booted by another entity and waiting
> @@ -163,10 +163,9 @@ enum rproc_state {
>  	RPROC_SUSPENDED	= 1,
>  	RPROC_RUNNING	= 2,
>  	RPROC_CRASHED	= 3,
> -	RPROC_DELETED	= 4,
> -	RPROC_ATTACHED	= 5,
> -	RPROC_DETACHED	= 6,
> -	RPROC_LAST	= 7,
> +	RPROC_ATTACHED	= 4,
> +	RPROC_DETACHED	= 5,
> +	RPROC_LAST	= 6,
>  };
>  
>  /**
> @@ -261,6 +260,8 @@ enum rproc_features {
>   * @index: index of this rproc device
>   * @attach_work: workqueue for attaching rproc
>   * @crash_handler: workqueue for handling a crash
> + * @crash_handler_lock: serializes crash handler queueing and deletion
> + * @deleting: remoteproc deletion has begun
>   * @crash_cnt: crash counter
>   * @recovery_disabled: flag that state if recovery was disabled
>   * @max_notifyid: largest allocated notify id.
> @@ -305,6 +306,8 @@ struct rproc {
>  	int index;
>  	struct work_struct attach_work;
>  	struct work_struct crash_handler;
> +	spinlock_t crash_handler_lock;
> +	bool deleting;
>  	unsigned int crash_cnt;
>  	bool recovery_disabled;
>  	int max_notifyid;
> 
> -- 
> 2.53.0
> 

-- 
-Mukesh Ojha

Re: [PATCH 2/3] remoteproc: Prevent crash handling to race with rproc_del()

[Mukesh Ojha]

On Thu, Jul 23, 2026 at 12:09:52PM +0200, Konrad Dybcio wrote:
> On 7/23/26 5:52 AM, Bjorn Andersson wrote:
> > There's no synchronization between rproc_crash_handler_work() and
> > rproc_del(), as such it's possible for a driver to be removed while
> > crash-handler work is scheduled, or even executing - resulting in
> > use-after-free issues.
> 
> [...]
> 
> > +	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
> > +	if (READ_ONCE(rproc->deleting)) {
> > +		spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> > +		return;
> > +	}
> > +
> >  	/* Prevent suspend while the remoteproc is being recovered */
> >  	pm_stay_awake(rproc->dev.parent);
> > +	queue_work(rproc_recovery_wq, &rproc->crash_handler);
> > +	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> >  
> >  	dev_err(&rproc->dev, "crash detected in %s: type %s\n",
> >  		rproc->name, rproc_crash_to_string(type));
> 
> GPT reported that the rproc may be gone/deleting at the time of
> this print since it's outside the lock, so it's not a given that
> rproc->dev and rproc->name are valid (although the window is pretty
> slim..)

Yes, this can happen if rproc->deleting is false and queue the
work and other thread cancel the work, completes rproc_del() and
now this thread running dev_err()..

-- 
-Mukesh Ojha

Re: [PATCH 2/3] remoteproc: Prevent crash handling to race with rproc_del()

[Mukesh Ojha]

On Thu, Jul 23, 2026 at 08:17:06PM +0530, Mukesh Ojha wrote:
> On Thu, Jul 23, 2026 at 03:52:28AM +0000, Bjorn Andersson wrote:
> > There's no synchronization between rproc_crash_handler_work() and
> > rproc_del(), as such it's possible for a driver to be removed while
> > crash-handler work is scheduled, or even executing - resulting in
> > use-after-free issues.
> > 
> > To avoid this the scheduled work need to be cancelled and synchronized
> > against before the removal proceeds.
> > 
> > In order to ensure that this doesn't race with the reporting, and
> > thereby scheduling new work, a "deleting" flag is introduced. This is
> > similar to the RPROC_DELETE state that was introduced to ensure that
> > "start" didn't race with rproc_del(), but the existing mechanism can not
> > be used as it's valid to call rproc_report_crash() in atomic context -
> > and the "state" is protected by a mutex.
> > 
> > In the event that work is cancelled the pm_stay_awake() is left
> > unbalanced and need to be unrolled.
> > 
> > The blocking and cancelling of crash-handler work prior to the actual
> > rproc_shutdown() call does have the explicit side-effect that crashes
> > resulting from the shutdown process will not enter the crash-handling
> > path, and as such will not generate devcoredumps etc. Due to the
> > existing mutual exclusion between these code paths there's no concrete
> > reduction in functionality, but further work would be needed to handle
> > this case.
> > 
> > Assisted-by: OpenCode:GPT-5.5
> > Fixes: 8afd519c3470 ("remoteproc: add rproc_report_crash function to notify rproc crashes")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c  | 42 +++++++++++++++++++++++++++--------
> >  drivers/remoteproc/remoteproc_sysfs.c |  1 -
> >  include/linux/remoteproc.h            | 13 ++++++-----
> >  3 files changed, 41 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 3cd4570513c2..899d2058bc45 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1851,6 +1851,11 @@ int rproc_trigger_recovery(struct rproc *rproc)
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (READ_ONCE(rproc->deleting)) {
> > +		ret = -ENODEV;
> > +		goto unlock_mutex;
> > +	}
> > +
> >  	/* State could have changed before we got the mutex */
> >  	if (rproc->state != RPROC_CRASHED)
> >  		goto unlock_mutex;
> > @@ -1883,6 +1888,11 @@ static void rproc_crash_handler_work(struct work_struct *work)
> >  
> >  	mutex_lock(&rproc->lock);
> >  
> > +	if (READ_ONCE(rproc->deleting)) {
> > +		mutex_unlock(&rproc->lock);
> > +		goto out;
> > +	}
> > +
> >  	if (rproc->state == RPROC_CRASHED) {
> >  		/* handle only the first crash detected */
> >  		mutex_unlock(&rproc->lock);
> > @@ -1938,9 +1948,9 @@ int rproc_boot(struct rproc *rproc)
> >  		return ret;
> >  	}
> >  
> > -	if (rproc->state == RPROC_DELETED) {
> > +	if (READ_ONCE(rproc->deleting)) {
> >  		ret = -ENODEV;
> > -		dev_err(dev, "can't boot deleted rproc %s\n", rproc->name);
> > +		dev_err(dev, "can't boot deleting rproc %s\n", rproc->name);
> >  		goto unlock_mutex;
> >  	}
> >  
> > @@ -2533,8 +2543,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> >  	INIT_LIST_HEAD(&rproc->subdevs);
> >  	INIT_LIST_HEAD(&rproc->dump_segments);
> >  
> > -	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
> >  	INIT_WORK(&rproc->attach_work, rproc_attach_work);
> > +	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
> > +	spin_lock_init(&rproc->crash_handler_lock);
> >  
> >  	rproc->state = RPROC_OFFLINE;
> >  
> > @@ -2598,16 +2609,21 @@ EXPORT_SYMBOL(rproc_put);
> >   */
> >  int rproc_del(struct rproc *rproc)
> >  {
> > +	unsigned long flags;
> > +
> >  	if (!rproc)
> >  		return -EINVAL;
> >  
> > +	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
> > +	WRITE_ONCE(rproc->deleting, true);
> > +	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> > +
> > +	if (cancel_work_sync(&rproc->crash_handler))
> > +		pm_relax(rproc->dev.parent);
> > +
> >  	/* TODO: make sure this works with rproc->power > 1 */
> >  	rproc_shutdown(rproc);
> >  
> > -	mutex_lock(&rproc->lock);
> > -	rproc->state = RPROC_DELETED;
> > -	mutex_unlock(&rproc->lock);
> > -
> >  	rproc_delete_debug_dir(rproc);
> >  
> >  	/* the rproc is downref'ed as soon as it's removed from the klist */
> > @@ -2719,18 +2735,26 @@ EXPORT_SYMBOL(rproc_get_by_child);
> >   */
> >  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
> >  {
> > +	unsigned long flags;
> > +
> >  	if (!rproc) {
> >  		pr_err("NULL rproc pointer\n");
> >  		return;
> >  	}
> >  
> > +	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
> > +	if (READ_ONCE(rproc->deleting)) {
> > +		spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> > +		return;
> > +	}
> > +
> >  	/* Prevent suspend while the remoteproc is being recovered */
> >  	pm_stay_awake(rproc->dev.parent);
> > +	queue_work(rproc_recovery_wq, &rproc->crash_handler);
> 
> Do we really need this inside the spinlock ?

Ignore this, as this may be needed to get it cancelled from the other
path.

> 
> > +	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> >  
> >  	dev_err(&rproc->dev, "crash detected in %s: type %s\n",
> >  		rproc->name, rproc_crash_to_string(type));
> > -
> > -	queue_work(rproc_recovery_wq, &rproc->crash_handler);
> >  }
> >  EXPORT_SYMBOL(rproc_report_crash);
> >  
> > diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> > index 138e752c5e4e..925b0cdbe577 100644
> > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > @@ -168,7 +168,6 @@ static const char * const rproc_state_string[] = {
> >  	[RPROC_SUSPENDED]	= "suspended",
> >  	[RPROC_RUNNING]		= "running",
> >  	[RPROC_CRASHED]		= "crashed",
> > -	[RPROC_DELETED]		= "deleted",
> >  	[RPROC_ATTACHED]	= "attached",
> >  	[RPROC_DETACHED]	= "detached",
> >  	[RPROC_LAST]		= "invalid",
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 17ed75a11e15..ef711a5b1a7f 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -37,6 +37,7 @@
> >  
> >  #include <linux/types.h>
> >  #include <linux/mutex.h>
> > +#include <linux/spinlock.h>
> >  #include <linux/virtio.h>
> >  #include <linux/cdev.h>
> >  #include <linux/completion.h>
> > @@ -145,7 +146,6 @@ struct rproc_ops {
> >   *			a message.
> >   * @RPROC_RUNNING:	device is up and running
> >   * @RPROC_CRASHED:	device has crashed; need to start recovery
> > - * @RPROC_DELETED:	device is deleted
> >   * @RPROC_ATTACHED:	device has been booted by another entity and the core
> >   *			has attached to it
> >   * @RPROC_DETACHED:	device has been booted by another entity and waiting
> > @@ -163,10 +163,9 @@ enum rproc_state {
> >  	RPROC_SUSPENDED	= 1,
> >  	RPROC_RUNNING	= 2,
> >  	RPROC_CRASHED	= 3,
> > -	RPROC_DELETED	= 4,
> > -	RPROC_ATTACHED	= 5,
> > -	RPROC_DETACHED	= 6,
> > -	RPROC_LAST	= 7,
> > +	RPROC_ATTACHED	= 4,
> > +	RPROC_DETACHED	= 5,
> > +	RPROC_LAST	= 6,
> >  };
> >  
> >  /**
> > @@ -261,6 +260,8 @@ enum rproc_features {
> >   * @index: index of this rproc device
> >   * @attach_work: workqueue for attaching rproc
> >   * @crash_handler: workqueue for handling a crash
> > + * @crash_handler_lock: serializes crash handler queueing and deletion
> > + * @deleting: remoteproc deletion has begun
> >   * @crash_cnt: crash counter
> >   * @recovery_disabled: flag that state if recovery was disabled
> >   * @max_notifyid: largest allocated notify id.
> > @@ -305,6 +306,8 @@ struct rproc {
> >  	int index;
> >  	struct work_struct attach_work;
> >  	struct work_struct crash_handler;
> > +	spinlock_t crash_handler_lock;
> > +	bool deleting;
> >  	unsigned int crash_cnt;
> >  	bool recovery_disabled;
> >  	int max_notifyid;
> > 
> > -- 
> > 2.53.0
> > 
> 
> -- 
> -Mukesh Ojha

-- 
-Mukesh Ojha

Re: [PATCH 3/3] remoteproc: Force shutdown during device removal

[Mukesh Ojha]

On Thu, Jul 23, 2026 at 12:54:58PM +0200, Konrad Dybcio wrote:
> On 7/23/26 5:52 AM, Bjorn Andersson wrote:
> > rproc_del() is subjected to the reference counted shutdown path, as such
> > a remoteproc with more than one reference will be left running as its
> > resources are released underneath it.
> > 
> > Refactor the shutdown path such that the reference count value is
> > ignored when called from rproc_del().
> > 
> > Assisted-by: OpenCode:GPT-5.5
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> 
> [...]
> 
> > +static int __rproc_shutdown(struct rproc *rproc, bool force)
> >  {
> >  	struct device *dev = &rproc->dev;
> >  	bool crashed;
> > @@ -2029,9 +2008,10 @@ int rproc_shutdown(struct rproc *rproc)
> >  	}
> >  	crashed = rproc->state == RPROC_CRASHED;
> >  
> > -	/* if the remote proc is still needed, bail out */
> > -	if (!atomic_dec_and_test(&rproc->power))
> > +	if (!atomic_dec_and_test(&rproc->power) && !force) {
> > +		/* The remote processor is still needed by another user. */
> >  		goto out;
> > +	}
> 
> GPT makes some unclear suggestions, effectively saying that rproc_shutdown(),
> unlike other paths, seems not to first check whether the rproc is deleting
> if the shutdown isn't forced. I think it's a valid concern.

Is it not indirectly the same thing: `rproc_del()` -> `__rproc_shutdown()`,
meaning `->deleting` is already set?

Or are you saying it is better to call `__rproc_shutdown(rproc,
rproc->deleting)`?
-- 
-Mukesh Ojha

Re: [PATCH 3/3] remoteproc: Force shutdown during device removal

[Bjorn Andersson]

On Thu, Jul 23, 2026 at 12:54:58PM +0200, Konrad Dybcio wrote:
> On 7/23/26 5:52 AM, Bjorn Andersson wrote:
> > rproc_del() is subjected to the reference counted shutdown path, as such
> > a remoteproc with more than one reference will be left running as its
> > resources are released underneath it.
> > 
> > Refactor the shutdown path such that the reference count value is
> > ignored when called from rproc_del().
> > 
> > Assisted-by: OpenCode:GPT-5.5
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> 
> [...]
> 
> > +static int __rproc_shutdown(struct rproc *rproc, bool force)
> >  {
> >  	struct device *dev = &rproc->dev;
> >  	bool crashed;
> > @@ -2029,9 +2008,10 @@ int rproc_shutdown(struct rproc *rproc)
> >  	}
> >  	crashed = rproc->state == RPROC_CRASHED;
> >  
> > -	/* if the remote proc is still needed, bail out */
> > -	if (!atomic_dec_and_test(&rproc->power))
> > +	if (!atomic_dec_and_test(&rproc->power) && !force) {
> > +		/* The remote processor is still needed by another user. */
> >  		goto out;
> > +	}
> 
> GPT makes some unclear suggestions, effectively saying that rproc_shutdown(),
> unlike other paths, seems not to first check whether the rproc is deleting
> if the shutdown isn't forced. I think it's a valid concern.
> 

I believe you're saying that there's nothing preventing a call to
rproc_shutdown() from racing with rproc_del()?

As far as I can tell, it's possible that while rproc_del() has set
`deleting` but hasn't yet called __rproc_shutdown() another reference
holder calls rproc_shutdown().

In the event that state is RUNNING or CRASHED that caller will stop the
remoteproc under mutual exclusion and leave it in OFFLINE, then let
the rproc_del() context proceed to find that the rproc is not RUNNING or
CRASHED and will as such return an error which is ignored.

If the contexts arrives to the mutual exclusion zone in opposite order,
the secondary caller will find the rproc in OFFLINE and fail the
rproc_shutdown() call.

So, I'm not able to see why we'd need to also guard against `deleting`.


That said, and as I stated in the cover-letter, there's more dragons
hidden in these code paths.

Regards,
Bjorn

> Konrad

Re: [PATCH 2/3] remoteproc: Prevent crash handling to race with rproc_del()

[Bjorn Andersson]

On Thu, Jul 23, 2026 at 12:09:52PM +0200, Konrad Dybcio wrote:
> On 7/23/26 5:52 AM, Bjorn Andersson wrote:
> > There's no synchronization between rproc_crash_handler_work() and
> > rproc_del(), as such it's possible for a driver to be removed while
> > crash-handler work is scheduled, or even executing - resulting in
> > use-after-free issues.
> 
> [...]
> 
> > +	spin_lock_irqsave(&rproc->crash_handler_lock, flags);
> > +	if (READ_ONCE(rproc->deleting)) {
> > +		spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> > +		return;
> > +	}
> > +
> >  	/* Prevent suspend while the remoteproc is being recovered */
> >  	pm_stay_awake(rproc->dev.parent);
> > +	queue_work(rproc_recovery_wq, &rproc->crash_handler);
> > +	spin_unlock_irqrestore(&rproc->crash_handler_lock, flags);
> >  
> >  	dev_err(&rproc->dev, "crash detected in %s: type %s\n",
> >  		rproc->name, rproc_crash_to_string(type));
> 
> GPT reported that the rproc may be gone/deleting at the time of
> this print since it's outside the lock, so it's not a given that
> rproc->dev and rproc->name are valid (although the window is pretty
> slim..)
> 

rproc->dev will be a valid object as long as the caller is holding a
reference to the rproc device (explicitly through rproc_get_by_phandle()
or implicitly through the device hierarchy), so there's no problem
there.

rproc->name has two different life cycles:

1) rproc_alloc(name) with name living on the heap or stack, will result
in `name` being owned by the rproc->dev life cycle and hence there's no
problem - as long as adequate references are held.

2) rproc_alloc(name) with name being a constant, here the rproc context
will hold a reference to the name in constant memory, and rmmod might
in this window have unmapped that constant.

So, the case when `name` is referencing a constant is a possible
problem, but unrelated to this patch.

Regards,
Bjorn