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
