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

From: Zhihao Cheng
Date: Thu Aug 06 2020 - 22:18:49 EST

在 2020/8/7 4:15, 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.
Never mind, we're all trying to figure it out.  :-) . Besides, I'm not good at expressing question in English. (In Practicing...)
The ubi thread can be reduced to a loop like this one:
1. for (;;) {
2. if (kthread_should_stop())
3. break;
5. if ( /* no work pending*/ ){
6. set_current_state(TASK_INTERRUPTIBLE);
7. schedule();
8. continue;
9. }
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.

Yes, but UBIFS is the exception, my solution looks like UBIFS.

int ubifs_bg_thread(void *info)
    while(1) {
        if (kthread_should_stop())

        if (!c->need_bgt) {
             * Nothing prevents us from going sleep now and
             * be never woken up and block the task which
             * could wait in 'kthread_stop()' forever.
            if (kthread_should_stop())

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

Your patch changes the loop as follows:
1. for (;;) {
2. if (kthread_should_stop())
3. break;
5. if ( /* no work pending*/ ){
6. set_current_state(TASK_INTERRUPTIBLE);
8. if (kthread_should_stop()) {
9. set_current_state(TASK_RUNNING);
10. break;
11. }
13. schedule();
14. continue;
15. }
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.
There's no disagreement so far.
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.

That's where we hold different views. I have 3 viewpoints(You can point out which one you disagree.):

1. If kthread_stop() happens at line 12, ubi thread is *marked* with stop flag, it will stop at kthread_should_stop() as long as it can reach the next iteration.

2. If task A is on runqueue and its state is TASK_RUNNING, task A will be scheduled to execute.

3. If kthread_stop() happens at line 12, after program counter going to line 14, ubi thead is on runqueue and its state is TASK_RUNNING. I have explained this in situation 1 in last session.

I mean ubi thread is on runqueue with TASK_RUNNING state & stop flag after the process you described.

Line 12   kthread_stop()

                 set_bit(mark stop flag) && wake_up_process(enqueue && set TASK_RUNNING )    => TASK_RUNNING & stop flag & on runqueue

Line 13  schedule()

                 Do nothing but pick next task to execute

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