From: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> mainline inclusion from mainline-v7.3-rc1 commit 74ee3b2f5767447c57959994341e5b95f1079977 category: bugfix bugzilla: https://atomgit.com/openeuler/kernel/issues/9905 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... -------------------------------- 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> Reviewed-by: Pradnya Dahiwale <pradnya.dahiwale@oss.qualcomm.com> Link: https://lore.kernel.org/r/20260723-rproc-rmmod-not-crashing-v1-2-546dfd5de0e... Signed-off-by: Bjorn Andersson <andersson@kernel.org> Conflicts: drivers/remoteproc/remoteproc_core.c include/linux/remoteproc.h [The downstream 6.6 tree has no struct rproc::attach_work field nor rproc_attach_work(); the conflict was resolved by keeping only INIT_WORK() for crash_handler and adding spin_lock_init() for the new crash_handler_lock, dropping the upstream attach_work init line.] Signed-off-by: Chen Jinghuang <chenjinghuang2@huawei.com> --- drivers/remoteproc/remoteproc_core.c | 40 +++++++++++++++++++++------ drivers/remoteproc/remoteproc_sysfs.c | 1 - include/linux/remoteproc.h | 13 +++++---- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 2d4ae3b5af86..1c489b757419 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1837,6 +1837,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; @@ -1869,6 +1874,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); @@ -1924,9 +1934,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; } @@ -2497,6 +2507,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, INIT_LIST_HEAD(&rproc->dump_segments); INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work); + spin_lock_init(&rproc->crash_handler_lock); rproc->state = RPROC_OFFLINE; @@ -2556,16 +2567,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 */ @@ -2677,18 +2693,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 8c7ea8922638..23de0490ba16 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 b4795698d8c2..e1634b9c7992 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> @@ -412,7 +413,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 @@ -430,10 +430,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, }; /** @@ -527,6 +526,8 @@ enum rproc_features { * @notifyids: idr for dynamically assigning rproc-wide unique notify ids * @index: index of this rproc device * @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. @@ -569,6 +570,8 @@ struct rproc { struct idr notifyids; int index; struct work_struct crash_handler; + spinlock_t crash_handler_lock; + bool deleting; unsigned int crash_cnt; bool recovery_disabled; int max_notifyid; -- 2.34.1