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

[Richard Weinberger]

On Wed, Aug 5, 2020 at 4:23 AM Zhihao Cheng <chengzhihao1@xxxxxxxxxx> wrote:
> Er, I can't get the point. I can list two possible situations, did I
> miss other situations?

Yes. You keep ignoring the case I brought up.

Let's start from scratch, maybe I miss something.
So I'm sorry for being persistent.

The ubi thread can be reduced to a loop like this one:
1. for (;;) {
2.      if (kthread_should_stop())
3.              break;
4.
5.      if ( /* no work pending*/ ){
6.              set_current_state(TASK_INTERRUPTIBLE);
7.              schedule();
8.              continue;
9.      }
10.
11.     do_work();
12. }

syzcaller found a case where stopping the thread did not work.
If another task tries to stop the thread while no work is pending and
the program counter in the thread
is between lines 5 and 6, the kthread_stop() instruction has no effect.
It has no effect because the thread sets the thread state to
interruptible sleep and then schedules away.

This is a common anti-pattern in the Linux kernel, sadly.

Do you agree with me so far or do you think syzcaller found a different issue?

Your patch changes the loop as follows:
1. for (;;) {
2.      if (kthread_should_stop())
3.              break;
4.
5.      if ( /* no work pending*/ ){
6.              set_current_state(TASK_INTERRUPTIBLE);
7.
8.              if (kthread_should_stop()) {
9.                      set_current_state(TASK_RUNNING);
10.                     break;
11.             }
12.
13.             schedule();
14.             continue;
15.     }
16.
17.     do_work();
18. }

That way there is a higher chance that the thread sees the stop flag
and gracefully terminates, I fully agree on that.
But it does not completely solve the problem.
If kthread_stop() happens while the program counter of the ubi thread
is at line 12, the stop flag is still missed
and we end up in interruptible sleep just like before.

So, to solve the problem entirely I suggest changing schedule() to
schedule_timeout() and let the thread wake up
periodically.

-- 
Thanks,
//richard