Messages in this thread | | | From | Suren Baghdasaryan <> | Date | Thu, 20 May 2021 19:11:08 -0700 | Subject | Re: [PATCH v6] psi: fix race between psi_trigger_create/destroy |
| |
On Thu, May 20, 2021 at 7:07 PM Huangzhaoyang <huangzhaoyang@gmail.com> wrote: > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > 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. > > Fixes: 461daba06bdc ("psi: eliminate kthread_worker from psi trigger > scheduling mechanism") > > Signed-off-by: ziwei.dai <ziwei.dai@unisoc.com> > Signed-off-by: ke.wang <ke.wang@unisoc.com> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
Looks good. Thanks! Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> --- > v2: change del_timer_sync to del_timer in psi_trigger_destroy > v3: remove timer_setup within psi_tirgger_create > protect del_timer by extending the critical section of mutex_lock > v4: amend fix information on comment > v5: delete the poll_timer while assigning the task to NULL > v6: update subject and comments as Suren's suggestion > --- > --- > kernel/sched/psi.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > index cc25a3c..58b36d1 100644 > --- a/kernel/sched/psi.c > +++ b/kernel/sched/psi.c > @@ -182,6 +182,8 @@ struct psi_group psi_system = { > > static void psi_avgs_work(struct work_struct *work); > > +static void poll_timer_fn(struct timer_list *t); > + > static void group_init(struct psi_group *group) > { > int cpu; > @@ -201,6 +203,8 @@ static void group_init(struct psi_group *group) > memset(group->polling_total, 0, sizeof(group->polling_total)); > group->polling_next_update = ULLONG_MAX; > group->polling_until = 0; > + init_waitqueue_head(&group->poll_wait); > + timer_setup(&group->poll_timer, poll_timer_fn, 0); > rcu_assign_pointer(group->poll_task, NULL); > } > > @@ -1157,9 +1161,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group, > return ERR_CAST(task); > } > atomic_set(&group->poll_wakeup, 0); > - init_waitqueue_head(&group->poll_wait); > wake_up_process(task); > - timer_setup(&group->poll_timer, poll_timer_fn, 0); > rcu_assign_pointer(group->poll_task, task); > } > > @@ -1211,6 +1213,7 @@ static void psi_trigger_destroy(struct kref *ref) > group->poll_task, > lockdep_is_held(&group->trigger_lock)); > rcu_assign_pointer(group->poll_task, NULL); > + del_timer(&group->poll_timer); > } > } > > @@ -1223,17 +1226,14 @@ static void psi_trigger_destroy(struct kref *ref) > */ > synchronize_rcu(); > /* > - * Destroy the kworker after releasing trigger_lock to prevent a > + * Stop kthread 'psimon' after releasing trigger_lock to prevent a > * deadlock while waiting for psi_poll_work to acquire trigger_lock > */ > if (task_to_destroy) { > /* > * After the RCU grace period has expired, the worker > * can no longer be found through group->poll_task. > - * But it might have been already scheduled before > - * that - deschedule it cleanly before destroying it. > */ > - del_timer_sync(&group->poll_timer); > kthread_stop(task_to_destroy); > } > kfree(t); > -- > 1.9.1 >
| |