[PATCH] ubi: check kthread_should_stop() after the setting of task state

[Richard Weinberger]

On Tue, Aug 4, 2020 at 4:58 AM Zhihao Cheng <chengzhihao1 at huawei.com> wrote:
> Oh, you're thinking about influence by schedule(), I get it. But I think
> it still works. Because the ubi_thread is still on runqueue, it will be
> scheduled to execute later anyway.

It will not get woken. This is the problem.

>
> op                                                    state of
> ubi_thread           on runqueue
> set_current_state(TASK_INTERRUPTIBLE) TASK_INTERRUPTIBLE              Yes
> if (kthread_should_stop()) // not satisfy
> TASK_INTERRUPTIBLE              Yes
> kthread_stop:
>    wake_up_process
>      ttwu_queue
>        ttwu_do_activate
>          ttwu_do_wakeup TASK_RUNNING                       Yes
> schedule
>    __schedule(false)
>
>   // prev->state is TASK_RUNNING, so we cannot move it from runqueue by
> deactivate_task(). So just pick next task to execute, ubi_thread is
> still on runqueue and will be scheduled to execute later.

It will be in state TASK_RUNNING only if your check is reached.

If kthread_stop() is called *before* your code:
+                       if (kthread_should_stop()) {
+                               set_current_state(TASK_RUNNING);
+                               break;
+                       }

...everything is fine.
But there is still a race window between your if
(kthread_should_stop()) and schedule() in the next line.
So if kthread_stop() is called right *after* the if and *before*
schedule(), the task state is still TASK_INTERRUPTIBLE
--> schedule() will not return unless the task is explicitly woken,
which does not happen.

Before your patch, the race window was much larger, I fully agree, but
your patch does not cure the problem
it just makes it harder to hit.

And using mdelay() to verify such a thing is also tricky because
mdelay() will influence the task state.

-- 
Thanks,
//richard