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

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 <[email protected]>
> ---
>  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