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

[Suren Baghdasaryan]

On Mon, May 17, 2021 at 12:33 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Mon, May 17, 2021 at 11:36 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> >
> > CC Suren
>
> Thanks!
>
> >
> > 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.
>
> resource of psi_system will not be reinitialized because
> init_waitqueue_head(&group->poll_wait) and friends are initialized
> only during the creation of the first trigger for that group (see this
> condition: https://elixir.bootlin.com/linux/latest/source/kernel/sched/psi.c#L1119).
>
> > >
> > > 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
>
> Could you please clarify this race a bit? I'm having trouble
> deciphering this diagram. I'm guessing psimon_new/psimon_old refer to
> a new trigger being created while an old one is being deleted, so it
> seems like a race between psi_trigger_create/psi_trigger_destroy. The
> combination of trigger_lock and RCU should be protecting us from that
> but maybe I missed something?
> I'm excluding a possibility of a race between psi_trigger_create with
> another existing trigger on the same group because the codepath
> calling init_waitqueue_head(&group->poll_wait) happens only when the
> first trigger for that group is created. Therefore if there is an
> existing trigger in that group that codepath will not be taken.

Ok, looking at the current code I think you can hit the following race
when psi_trigger_destroy is destroying the last trigger in a psi group
while racing with psi_trigger_create:

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.
Let me think if we can handle this without moving initialization into
group_init().

>
> > >
> > > 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.
>
> Well, this means we initialize resources for triggers in each psi
> group even if the user never creates any triggers. Current logic
> initializes them when the first trigger in the group gets created.
>
> >
> > >       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?
>
> I would like to get more details about this race before trying to fix
> it. Please clarify.
> Thanks!