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: <CAFLxGvzwoC1GcjJOfwpc8V5LD79=8XiJaNV2HjOm8EdQcCbp+w@mail.gmail.com>
Date:   Thu, 6 Aug 2020 22:15:46 +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 Wed, Aug 5, 2020 at 4:23 AM Zhihao Cheng <chengzhihao1@...wei.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.

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.

Do you agree with me so far or do you think syzcaller found a different issue?

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

So, to solve the problem entirely I suggest changing schedule() to
schedule_timeout() and let the thread wake up
periodically.

-- 
Thanks,
//richard