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
