Messages in this thread Patch in this message | | | From | Cristian Marussi <> | Subject | [PATCH] firmware: arm_scmi: fix notifications locking | Date | Tue, 13 Oct 2020 14:31:09 +0100 |
| |
When a protocol registers its events the notification core takes care to re-scan the hashtable of pending event handlers and activate all the possibly existent handlers that refer to any of the events just registered by the new protocol; when a pending handler becomes active the core takes also care to ask the SCMI platform to enable the corresponding events' notifications in the SCMI firmware.
If, for whatever reason, the enable fails such invalid event handler must be finally removed and freed but it must be treated as an active handler like it has just become: ensure to use the scmi_put_active_handler() helper which handles properly the needed additional mutexing.
Failing to properly acquire all the needed mutexes exposes a race that leads to the following splat being observed:
[ 212.840876] ------------[ cut here ]------------ [ 212.845569] refcount_t: underflow; use-after-free. [ 212.850544] WARNING: CPU: 0 PID: 388 at lib/refcount.c:28 refcount_warn_saturate+0xf8/0x148 [ 212.858913] Modules linked in: dummy_scmi_consumer(-) scmi_perf [last unloaded: scmi_cpufreq] [ 212.867478] CPU: 0 PID: 388 Comm: rmmod Tainted: G W 5.9.0-rc1-00020-g9c4395e7867d-dirty #4 [ 212.877153] Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jun 30 2020 [ 212.887963] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--) [ 212.893554] pc : refcount_warn_saturate+0xf8/0x148 [ 212.898361] lr : refcount_warn_saturate+0xf8/0x148 [ 212.903160] sp : ffff80001298bc00 [ 212.906480] x29: ffff80001298bc00 x28: ffff00097470aac0 [ 212.911807] x27: 0000000000000000 x26: 0000000000000000 [ 212.917135] x25: ffff00097357fc80 x24: 0000000000000002 [ 212.922462] x23: ffff00097357fca8 x22: 0000000000000003 [ 212.927790] x21: ffff000974474688 x20: ffff000971e04f00 [ 212.933117] x19: 0000000000000001 x18: 0000000000000010 [ 212.938444] x17: 0000000000000000 x16: 0000000000000000 [ 212.943771] x15: ffffffffffffffff x14: ffff800012269948 [ 212.949098] x13: ffff80009298b947 x12: ffff80001298b94f [ 212.954426] x11: 0001000000000000 x10: 0000000000000a10 [ 212.959753] x9 : ffff80001039fe88 x8 : ffff00097470b530 [ 212.965080] x7 : 00000000ffffffff x6 : ffff00097ef1b200 [ 212.970407] x5 : ffff00097ef1b200 x4 : 0000000000000000 [ 212.975734] x3 : ffff00097ef2a398 x2 : 0000000000000001 [ 212.981060] x1 : 4b16c3af5d721600 x0 : 0000000000000000 [ 212.986388] Call trace: [ 212.988847] refcount_warn_saturate+0xf8/0x148 [ 212.993308] scmi_put_handler_unlocked.isra.10+0x204/0x208 [ 212.998811] scmi_put_handler+0x50/0xa0 [ 213.002659] scmi_unregister_notifier+0x1bc/0x240 [ 213.007385] scmi_notify_tester_remove+0x4c/0x68 [dummy_scmi_consumer] [ 213.013931] scmi_dev_remove+0x54/0x68 [ 213.017695] device_release_driver_internal+0x114/0x1e8 [ 213.022934] driver_detach+0x58/0xe8 [ 213.026520] bus_remove_driver+0x88/0xe0 [ 213.030454] driver_unregister+0x38/0x68 [ 213.034389] scmi_driver_unregister+0x1c/0x28 [ 213.038763] scmi_drv_exit+0x1c/0xae0 [dummy_scmi_consumer] [ 213.044354] __arm64_sys_delete_module+0x1a4/0x268 [ 213.049160] el0_svc_common.constprop.3+0x94/0x178 [ 213.053963] do_el0_svc+0x2c/0x98 [ 213.057290] el0_sync_handler+0x148/0x1a8 [ 213.061310] el0_sync+0x158/0x180 [ 213.064632] ---[ end trace 5bff2d25d5820911 ]---
Fixes: e7c215f358a35 ("firmware: arm_scmi: Add notification callbacks-registration") Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> ---
Applied and tested on [1] on top of :
commit: 9724722fde8f ("firmware: arm_scmi: Add missing Rx size re-initialisation")
[1]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi --- --- drivers/firmware/arm_scmi/notify.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c index 2754f9d01636..c24e427dce0d 100644 --- a/drivers/firmware/arm_scmi/notify.c +++ b/drivers/firmware/arm_scmi/notify.c @@ -1403,15 +1403,21 @@ static void scmi_protocols_late_init(struct work_struct *work) "finalized PENDING handler - key:%X\n", hndl->key); ret = scmi_event_handler_enable_events(hndl); + if (ret) { + dev_dbg(ni->handle->dev, + "purging INVALID handler - key:%X\n", + hndl->key); + scmi_put_active_handler(ni, hndl); + } } else { ret = scmi_valid_pending_handler(ni, hndl); - } - if (ret) { - dev_dbg(ni->handle->dev, - "purging PENDING handler - key:%X\n", - hndl->key); - /* this hndl can be only a pending one */ - scmi_put_handler_unlocked(ni, hndl); + if (ret) { + dev_dbg(ni->handle->dev, + "purging PENDING handler - key:%X\n", + hndl->key); + /* this hndl can be only a pending one */ + scmi_put_handler_unlocked(ni, hndl); + } } } mutex_unlock(&ni->pending_mtx); -- 2.17.1
| |