Race detected between psi_trigger_destroy/create as shown below, which cause panic by accessing invalid psi_system->poll_wait->wait_queue_entry and psi_system->poll_timer->entry->next. Under this modification, the race window is removed by initialising poll_wait and poll_timer in group_init which are executed only once at beginning. psi_trigger_destroy() psi_trigger_create() mutex_lock(trigger_lock); rcu_assign_pointer(poll_task, NULL); mutex_unlock(trigger_lock); mutex_lock(trigger_lock); if (!rcu_access_pointer(group->poll_task)) { timer_setup(poll_timer, poll_timer_fn, 0); rcu_assign_pointer(poll_task, task); } mutex_unlock(trigger_lock); synchronize_rcu(); del_timer_sync(poll_timer); <-- poll_timer has been reinitialized by psi_trigger_create() So, trigger_lock/RCU correctly protects destruction of group->poll_task but misses this race affecting poll_timer and poll_wait.
Showing the changes to kernel (1 of 1 file). Show all
Unified · Split
| @@ -182,6 +182,8 @@ struct psi_group psi_system = { · 181 unmodified lines | |||
| 182 | 182 | ||
| 183 | static void psi_avgs_work(struct work_struct *work); | 183 | static void psi_avgs_work(struct work_struct *work); |
| 184 | 184 | ||
| 185 | static void poll_timer_fn(struct timer_list *t); | ||
| 186 | |||
| 185 | static void group_init(struct psi_group *group) | 187 | static void group_init(struct psi_group *group) |
| 186 | { | 188 | { |
| 187 | int cpu; | 189 | int cpu; |
| @@ -201,6 +203,8 @@ static void group_init(struct psi_group *group) · 13 unmodified lines | |||
| 201 | memset(group->polling_total, 0, sizeof(group->polling_total)); | 203 | memset(group->polling_total, 0, sizeof(group->polling_total)); |
| 202 | group->polling_next_update = ULLONG_MAX; | 204 | group->polling_next_update = ULLONG_MAX; |
| 203 | group->polling_until = 0; | 205 | group->polling_until = 0; |
| 206 | init_waitqueue_head(&group->poll_wait); | ||
| 207 | timer_setup(&group->poll_timer, poll_timer_fn, 0); | ||
| 204 | rcu_assign_pointer(group->poll_task, NULL); | 208 | rcu_assign_pointer(group->poll_task, NULL); |
| 205 | } | 209 | } |
| 206 | 210 | ||
| @@ -1157,9 +1161,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group, · 950 unmodified lines | |||
| 1157 | return ERR_CAST(task); | 1161 | return ERR_CAST(task); |
| 1158 | } | 1162 | } |
| 1159 | atomic_set(&group->poll_wakeup, 0); | 1163 | atomic_set(&group->poll_wakeup, 0); |
| 1160 | init_waitqueue_head(&group->poll_wait); | ||
| 1161 | wake_up_process(task); | 1164 | wake_up_process(task); |
| 1162 | timer_setup(&group->poll_timer, poll_timer_fn, 0); | ||
| 1163 | rcu_assign_pointer(group->poll_task, task); | 1165 | rcu_assign_pointer(group->poll_task, task); |
| 1164 | } | 1166 | } |
| 1165 | 1167 | ||
| @@ -1211,6 +1213,7 @@ static void psi_trigger_destroy(struct kref *ref) · 45 unmodified lines | |||
| 1211 | group->poll_task, | 1213 | group->poll_task, |
| 1212 | lockdep_is_held(&group->trigger_lock)); | 1214 | lockdep_is_held(&group->trigger_lock)); |
| 1213 | rcu_assign_pointer(group->poll_task, NULL); | 1215 | rcu_assign_pointer(group->poll_task, NULL); |
| 1216 | del_timer(&group->poll_timer); | ||
| 1214 | } | 1217 | } |
| 1215 | } | 1218 | } |
| 1216 | 1219 | ||
| @@ -1223,17 +1226,14 @@ static void psi_trigger_destroy(struct kref *ref) · 6 unmodified lines | |||
| 1223 | */ | 1226 | */ |
| 1224 | synchronize_rcu(); | 1227 | synchronize_rcu(); |
| 1225 | /* | 1228 | /* |
| 1226 | * Destroy the kworker after releasing trigger_lock to prevent a | 1229 | * Stop kthread 'psimon' after releasing trigger_lock to prevent a |
| 1227 | * deadlock while waiting for psi_poll_work to acquire trigger_lock | 1230 | * deadlock while waiting for psi_poll_work to acquire trigger_lock |
| 1228 | */ | 1231 | */ |
| 1229 | if (task_to_destroy) { | 1232 | if (task_to_destroy) { |
| 1230 | /* | 1233 | /* |
| 1231 | * After the RCU grace period has expired, the worker | 1234 | * After the RCU grace period has expired, the worker |
| 1232 | * can no longer be found through group->poll_task. | 1235 | * can no longer be found through group->poll_task. |
| 1233 | * But it might have been already scheduled before | ||
| 1234 | * that - deschedule it cleanly before destroying it. | ||
| 1235 | */ | 1236 | */ |
| 1236 | del_timer_sync(&group->poll_timer); | ||
| 1237 | kthread_stop(task_to_destroy); | 1237 | kthread_stop(task_to_destroy); |
| 1238 | } | 1238 | } |
| 1239 | kfree(t); | 1239 | kfree(t); |