Messages in this thread | | | Subject | Re: [PATCH] ubi: check kthread_should_stop() after the setting of task state | From | Zhihao Cheng <> | Date | Fri, 7 Aug 2020 10:18:37 +0800 |
| |
在 2020/8/7 4:15, Richard Weinberger 写道: > On Wed, Aug 5, 2020 at 4:23 AM Zhihao Cheng <chengzhihao1@huawei.com> 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. >
| |