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

From: Richard Weinberger
Date: Tue Aug 04 2020 - 17:56:27 EST

On Tue, Aug 4, 2020 at 4:58 AM Zhihao Cheng <chengzhihao1@xxxxxxxxxx> 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
> if (kthread_should_stop()) // not satisfy
> 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.