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

[Zhihao Cheng]

在 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;
> 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.

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

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

        set_current_state(TASK_INTERRUPTIBLE);
        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())
                break;
            schedule();
            continue;
        }
    }
}


> 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;
> 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.
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
> periodically.