lkml.org 
[lkml]   [2020]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] ubi: check kthread_should_stop() after the setting of task state
From
Date
在 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.
>


\
 
 \ /
  Last update: 2020-08-07 04:18    [W:0.077 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site