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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2086f822-e67a-43e4-76d8-5339eaccd3ac@huawei.com>
Date:   Wed, 5 Aug 2020 10:23:17 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Richard Weinberger <richard.weinberger@...il.com>
CC:     <linux-mtd@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Richard Weinberger <richard@....at>,
        "zhangyi (F)" <yi.zhang@...wei.com>
Subject: Re: [PATCH] ubi: check kthread_should_stop() after the setting of
 task state

在 2020/8/5 5:56, Richard Weinberger 写道:
> On Tue, Aug 4, 2020 at 4:58 AM Zhihao Cheng <chengzhihao1@...wei.com> wrote:
>> Oh, you're thinking about influence by schedule(), I get it. But I think
>> it still works. Because the ubi_thread is still on runqueue, it will be
>> scheduled to execute later anyway.
> It will not get woken. This is the problem.
>
>> op                                                    state of
>> ubi_thread           on runqueue
>> set_current_state(TASK_INTERRUPTIBLE) TASK_INTERRUPTIBLE              Yes
>> if (kthread_should_stop()) // not satisfy
>> TASK_INTERRUPTIBLE              Yes
>> kthread_stop:
>>     wake_up_process
>>       ttwu_queue
>>         ttwu_do_activate
>>           ttwu_do_wakeup TASK_RUNNING                       Yes
>> schedule
>>     __schedule(false)
>>
>>    // prev->state is TASK_RUNNING, so we cannot move it from runqueue by
>> deactivate_task(). So just pick next task to execute, ubi_thread is
>> still on runqueue and will be scheduled to execute later.
> It will be in state TASK_RUNNING only if your check is reached.
>
> If kthread_stop() is called *before* your code:
> +                       if (kthread_should_stop()) {
> +                               set_current_state(TASK_RUNNING);
> +                               break;
> +                       }
>
> ...everything is fine.
> But there is still a race window between your if
> (kthread_should_stop()) and schedule() in the next line.
> So if kthread_stop() is called right *after* the if and *before*
> schedule(), the task state is still TASK_INTERRUPTIBLE
> --> schedule() will not return unless the task is explicitly woken,
> which does not happen.
Er, I can't get the point. I can list two possible situations, did I 
miss other situations?

P1:ubi_thread
   set_current_state(TASK_INTERRUPTIBLE)
   if (kthread_should_stop()) {
     set_current_state(TASK_RUNNING)
     break
   }
   schedule()                            -> don't *remove* task from 
runqueue if *TASK_RUNNING*, removing operation is protected by rq_lock

P2:kthread_stop
   set_bit(KTHREAD_SHOULD_STOP, &kthread->flags)
   wake_up_process(k)             -> enqueue task & set *TASK_RUNNING*, 
these two operations are protected by rq_lock
   wait_for_completion(&kthread->exited)


Situation 1:
P1_set_current_state               on-rq, TASK_RUNNING -> TASK_INTERRUPTIBLE
P1_kthread_should_stop        on-rq, TASK_INTERRUPTIBLE
P2_set_bit                               on-rq, TASK_INTERRUPTIBLE , 
KTHREAD_SHOULD_STOP
P2_wake_up_process             on-rq, TASK_INTERRUPTIBLE -> TASK_RUNNING 
, KTHREAD_SHOULD_STOP
P1_schedule                           on-rq, TASK_RUNNING , 
KTHREAD_SHOULD_STOP
P2_wait_for_completion        // wait for P1 exit

Situation 2:
P1_set_current_state             on-rq, TASK_RUNNING -> TASK_INTERRUPTIBLE
P1_kthread_should_stop       on-rq, TASK_INTERRUPTIBLE
P2_set_bit                             on-rq, TASK_INTERRUPTIBLE , 
KTHREAD_SHOULD_STOP
P1_schedule                          off-rq, TASK_INTERRUPTIBLE , 
KTHREAD_SHOULD_STOP
P2_wake_up_process             on-rq, TASK_INTERRUPTIBLE -> TASK_RUNNING 
, KTHREAD_SHOULD_STOP
P2_wait_for_completion       // wait for P1 exit
> Before your patch, the race window was much larger, I fully agree, but
> your patch does not cure the problem
> it just makes it harder to hit.
>
> And using mdelay() to verify such a thing is also tricky because
> mdelay() will influence the task state.
>