Re: [[RFC]PATCH] psi: fix race between psi_trigger_create and psimon

[Johannes Weiner]

CC Suren

On Mon, May 17, 2021 at 05:04:09PM +0800, Huangzhaoyang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> 
> Race detected between psimon_new and psimon_old as shown below, which
> cause panic by accessing invalid psi_system->poll_wait->wait_queue_entry
> and psi_system->poll_timer->entry->next. It is not necessary to reinit
> resource of psi_system when psi_trigger_create.
> 
> psi_trigger_create      psimon_new     psimon_old
>  init_waitqueue_head                    finish_wait
>                                           spin_lock(lock_old)
> 	spin_lock_init(lock_new)
>  wake_up_process(psimon_new)
> 
>                         finish_wait
>                           spin_lock(lock_new)
>                             list_del       list_del
> 
> Signed-off-by: ziwei.dai <ziwei.dai@xxxxxxxxxx>
> Signed-off-by: ke.wang <ke.wang@xxxxxxxxxx>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> ---
>  kernel/sched/psi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index cc25a3c..d00e585 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);

This makes sense.

>  	rcu_assign_pointer(group->poll_task, NULL);
>  }
>  
> @@ -1157,7 +1161,6 @@ 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);

This looks now unncessary?

>  		rcu_assign_pointer(group->poll_task, task);
> @@ -1233,7 +1236,6 @@ static void psi_trigger_destroy(struct kref *ref)
>  		 * But it might have been already scheduled before
>  		 * that - deschedule it cleanly before destroying it.
>  		 */
> -		del_timer_sync(&group->poll_timer);

And this looks wrong. Did you mean to delete the timer_setup() line
instead?