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

[Zhihao Cheng]

å 2020/8/3 5:25, Richard Weinberger åé:
> On Mon, Jun 1, 2020 at 11:13 AM Zhihao Cheng <chengzhihao1@xxxxxxxxxx> wrote:
> > A detach hung is possible when a race occurs between the detach process
> > and the ubi background thread. The following sequences outline the race:
> >
> >    ubi thread: if (list_empty(&ubi->works)...
> >
> >    ubi detach: set_bit(KTHREAD_SHOULD_STOP, &kthread->flags)
> >                => by kthread_stop()
> >                wake_up_process()
> >                => ubi thread is still running, so 0 is returned
> >
> >    ubi thread: set_current_state(TASK_INTERRUPTIBLE)
> >                schedule()
> >                => ubi thread will never be scheduled again
> >
> >    ubi detach: wait_for_completion()
> >                => hung task!
> >
> > To fix that, we need to check kthread_should_stop() after we set the
> > task state, so the ubi thread will either see the stop bit and exit or
> > the task state is reset to runnable such that it isn't scheduled out
> > indefinitely.
> >
> > Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> > Cc: <Stable@xxxxxxxxxxxxxxx>
> > Fixes: 801c135ce73d5df1ca ("UBI: Unsorted Block Images")
> > Reported-by: syzbot+853639d0cb16c31c7a14@xxxxxxxxxxxxxxxxxxxxxxxxx
> > ---
> >   drivers/mtd/ubi/wl.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> > index 5146cce5fe32..a4d4343053d7 100644
> > --- a/drivers/mtd/ubi/wl.c
> > +++ b/drivers/mtd/ubi/wl.c
> > @@ -1628,6 +1628,19 @@ int ubi_thread(void *u)
> >                      !ubi->thread_enabled || ubi_dbg_is_bgt_disabled(ubi)) {
> >                          set_current_state(TASK_INTERRUPTIBLE);
> >                          spin_unlock(&ubi->wl_lock);
> > +
> > +                       /*
> > +                        * Check kthread_should_stop() after we set the task
> > +                        * state to guarantee that we either see the stop bit
> > +                        * and exit or the task state is reset to runnable such
> > +                        * that it's not scheduled out indefinitely and detects
> > +                        * the stop bit at kthread_should_stop().
> > +                        */
> > +                       if (kthread_should_stop()) {
> > +                               set_current_state(TASK_RUNNING);
> > +                               break;
> > +                       }
> > +
> Hmm, I see the problem but I fear this patch does not cure the race completely.
> It just lowers the chance to hit it.
> What if KTHREAD_SHOULD_STOP is set right after you checked for it?

The patch can handle this case. ubi_thread will exit at 
kthread_should_stop() in next iteration.


You can apply following patch to verify it. (You may set 
'kernel.hung_task_timeout_secs = 10' by sysctl)


diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 27636063ed1b..44047861c2a1 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -91,6 +91,7 @@
Â#include <linux/kthread.h>
Â#include "ubi.h"
Â#include "wl.h"
+#include <linux/delay.h>

Â/* Number of physical eraseblocks reserved for wear-leveling purposes */
Â#define WL_RESERVED_PEBS 1
@@ -1627,8 +1628,10 @@ int ubi_thread(void *u)
ÂÂÂÂÂÂÂ for (;;) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int err;

-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (kthread_should_stop())
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (kthread_should_stop()) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Exit at stop A\n");
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }

ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (try_to_freeze())
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
@@ -1638,6 +1641,15 @@ int ubi_thread(void *u)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ !ubi->thread_enabled || ubi_dbg_is_bgt_disabled(ubi)) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ set_current_state(TASK_INTERRUPTIBLE);
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ spin_unlock(&ubi->wl_lock);
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (kthread_should_stop()) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ set_current_state(TASK_RUNNING);
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Check should stop B\n");
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mdelay(5000);
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("delay 5000ms \n");
+
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ schedule();
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 132f84a5fde3..18627acb9573 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -590,6 +590,10 @@ int kthread_stop(struct task_struct *k)
ÂÂÂÂÂÂÂ get_task_struct(k);
ÂÂÂÂÂÂÂ kthread = to_kthread(k);
ÂÂÂÂÂÂÂ set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
+
+ÂÂÂÂÂÂ if (k->comm && !strncmp(k->comm, "ubi_bgt0d", strlen("ubi_bgt0d")))
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Stop flag has been set for task %s\n", k->comm);
+
ÂÂÂÂÂÂÂ kthread_unpark(k);
ÂÂÂÂÂÂÂ wake_up_process(k);
ÂÂÂÂÂÂÂ wait_for_completion(&kthread->exited);

kernel msg:
[Â 210.602005] Check should stop BÂÂÂÂÂÂÂÂÂÂÂÂ # Please execute 
ubi_detach manually when you see this
[Â 211.347638] Stop flag has been set for task ubi_bgt0d
[Â 215.603026] delay 5000ms
[Â 215.605728] Exit at stop A
> >                          schedule();
> >                          continue;
> >                  }