Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 17 May 2021 14:36:50 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Huangzhaoyang <huangzhaoyang@gmail.com>
Cc:     Zhaoyang Huang <zhaoyang.huang@unisoc.com>,
        Ziwei Dai <ziwei.dai@unisoc.com>, Ke Wang <ke.wang@unisoc.com>,
        Suren Baghdasaryan <surenb@google.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [[RFC]PATCH] psi: fix race between psi_trigger_create and psimon
Message-ID: <YKK3wgrhqzaiE7rQ@cmpxchg.org>
References: <1621242249-8314-1-git-send-email-huangzhaoyang@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1621242249-8314-1-git-send-email-huangzhaoyang@gmail.com>
Precedence: bulk

CC Suren

On Mon, May 17, 2021 at 05:04:09PM +0800, Huangzhaoyang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> 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@unisoc.com>
> Signed-off-by: ke.wang <ke.wang@unisoc.com>
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
>  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?