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

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 <[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 ?

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