Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

staging: vc04_services: Fix RCU dereference check

In service_callback path RCU dereferenced pointer struct vchiq_service
need to be accessed inside rcu read-critical section.

Also userdata/user_service part of vchiq_service is accessed around
different synchronization mechanism, getting an extra reference to a
pointer keeps sematics simpler and avoids prolonged graceperiod.

Accessing vchiq_service with rcu_read_[lock/unlock] fixes below issue.

[ 32.201659] =============================
[ 32.201664] WARNING: suspicious RCU usage
[ 32.201670] 5.15.11-rt24-v8+ #3 Not tainted
[ 32.201680] -----------------------------
[ 32.201685] drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h:529 suspicious rcu_dereference_check() usage!
[ 32.201695]
[ 32.201695] other info that might help us debug this:
[ 32.201695]
[ 32.201700]
[ 32.201700] rcu_scheduler_active = 2, debug_locks = 1
[ 32.201708] no locks held by vchiq-slot/0/98.
[ 32.201715]
[ 32.201715] stack backtrace:
[ 32.201723] CPU: 1 PID: 98 Comm: vchiq-slot/0 Not tainted 5.15.11-rt24-v8+ #3
[ 32.201733] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[ 32.201739] Call trace:
[ 32.201742] dump_backtrace+0x0/0x1b8
[ 32.201772] show_stack+0x20/0x30
[ 32.201784] dump_stack_lvl+0x8c/0xb8
[ 32.201799] dump_stack+0x18/0x34
[ 32.201808] lockdep_rcu_suspicious+0xe4/0xf8
[ 32.201817] service_callback+0x124/0x400
[ 32.201830] slot_handler_func+0xf60/0x1e20
[ 32.201839] kthread+0x19c/0x1a8
[ 32.201849] ret_from_fork+0x10/0x20

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
Link: https://lore.kernel.org/r/20211231195406.5479-1-treasure4paddy@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

authored by

Padmanabha Srinivasaiah and committed by
Greg Kroah-Hartman
0cea730c e783362e

+18 -2
+18 -2
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
··· 1058 1058 1059 1059 DEBUG_TRACE(SERVICE_CALLBACK_LINE); 1060 1060 1061 + rcu_read_lock(); 1061 1062 service = handle_to_service(handle); 1062 - if (WARN_ON(!service)) 1063 + if (WARN_ON(!service)) { 1064 + rcu_read_unlock(); 1063 1065 return VCHIQ_SUCCESS; 1066 + } 1064 1067 1065 1068 user_service = (struct user_service *)service->base.userdata; 1066 1069 instance = user_service->instance; 1067 1070 1068 - if (!instance || instance->closing) 1071 + if (!instance || instance->closing) { 1072 + rcu_read_unlock(); 1069 1073 return VCHIQ_SUCCESS; 1074 + } 1075 + 1076 + /* 1077 + * As hopping around different synchronization mechanism, 1078 + * taking an extra reference results in simpler implementation. 1079 + */ 1080 + vchiq_service_get(service); 1081 + rcu_read_unlock(); 1070 1082 1071 1083 vchiq_log_trace(vchiq_arm_log_level, 1072 1084 "%s - service %lx(%d,%p), reason %d, header %lx, instance %lx, bulk_userdata %lx", ··· 1109 1097 bulk_userdata); 1110 1098 if (status != VCHIQ_SUCCESS) { 1111 1099 DEBUG_TRACE(SERVICE_CALLBACK_LINE); 1100 + vchiq_service_put(service); 1112 1101 return status; 1113 1102 } 1114 1103 } ··· 1118 1105 if (wait_for_completion_interruptible(&user_service->remove_event)) { 1119 1106 vchiq_log_info(vchiq_arm_log_level, "%s interrupted", __func__); 1120 1107 DEBUG_TRACE(SERVICE_CALLBACK_LINE); 1108 + vchiq_service_put(service); 1121 1109 return VCHIQ_RETRY; 1122 1110 } else if (instance->closing) { 1123 1111 vchiq_log_info(vchiq_arm_log_level, "%s closing", __func__); 1124 1112 DEBUG_TRACE(SERVICE_CALLBACK_LINE); 1113 + vchiq_service_put(service); 1125 1114 return VCHIQ_ERROR; 1126 1115 } 1127 1116 DEBUG_TRACE(SERVICE_CALLBACK_LINE); ··· 1152 1137 header = NULL; 1153 1138 } 1154 1139 DEBUG_TRACE(SERVICE_CALLBACK_LINE); 1140 + vchiq_service_put(service); 1155 1141 1156 1142 if (skip_completion) 1157 1143 return VCHIQ_SUCCESS;