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]

Date:   Tue, 4 Aug 2020 23:56:13 +0200
From:   Richard Weinberger <richard.weinberger@...il.com>
To:     Zhihao Cheng <chengzhihao1@...wei.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

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.

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.

-- 
Thanks,
//richard