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;
MIME-Version: 1.0
References: <1623371374-15664-1-git-send-email-huangzhaoyang@gmail.com>
In-Reply-To: <1623371374-15664-1-git-send-email-huangzhaoyang@gmail.com>
From:   Zhaoyang Huang <huangzhaoyang@gmail.com>
Date:   Fri, 11 Jun 2021 08:37:05 +0800
Message-ID: <CAGWkznH34=fTybuJyd0H6ceB48EKLT77m4C2QkRNS5LhCc6h3w@mail.gmail.com>
Subject: [Resend PATCH v6] psi: fix race between psi_trigger_create/destroy
To:     Johannes Weiner <hannes@cmpxchg.org>,
        Suren Baghdasaryan <surenb@google.com>,
        Zhaoyang Huang <zhaoyang.huang@unisoc.com>,
        Ziwei Dai <ziwei.dai@unisoc.com>, Ke Wang <ke.wang@unisoc.com>,
        LKML <linux-kernel@vger.kernel.org>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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")

Co-developed-by: ziwei.dai <ziwei.dai@unisoc.com>
Signed-off-by: ziwei.dai <ziwei.dai@unisoc.com>
Co-developed-by: ke.wang <ke.wang@unisoc.com>
Signed-off-by: ke.wang <ke.wang@unisoc.com>
Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.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