Re: [PATCH 2/2] sched: do not call workqueue sleep hook if task is already dead

[Oleg Nesterov]

On 10/20, Andy Lutomirski wrote:
>
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -3380,8 +3380,22 @@ static void __sched notrace __schedule(bool preempt)
> >                          * If a worker went to sleep, notify and ask 
> > workqueue
> >                          * whether it wants to wake up a task to maintain
> >                          * concurrency.
> > +                        *
> > +                        * Also the following stack is possible:
> > +                        *    oops_end()
> > +                        *    do_exit()
> > +                        *    schedule()
> > +                        *
> > +                        * If panic_on_oops is not set and oops happens on
> > +                        * a workqueue execution path, thread will be 
> > killed.
> > +                        * That is definitly sad, but not to make the 
> > situation
> > +                        * even worse we have to ignore dead tasks in order 
> > not
> > +                        * to step on zeroed out members (e.g. 
> > t->vfork_done is
> > +                        * already NULL on that path, since we were called 
> > by
> > +                        * do_exit()))

And we have more problems like this. Say, if blk_flush_plug_list()
crashes it will likely crash again and again recursively.

> >                          */
> > -                       if (prev->flags & PF_WQ_WORKER) {
> > +                       if (prev->flags & PF_WQ_WORKER &&
> > +                           prev->state != TASK_DEAD) {

I don't think we should change __schedule()... Can't we simply clear
PF_WQ_WORKER in complete_vfork_done() ? Or add the PF_EXITING checks
into wq_worker_sleeping() and wq_worker_waking_up().

Or perhaps something like the change below.

Oleg.

--- x/kernel/workqueue.c
+++ x/kernel/workqueue.c
@@ -2157,6 +2157,14 @@ static void process_scheduled_works(stru
        }
 }
 
+static void oops_handler(struct callback_head *oops_work)
+{
+       if (!(current->flags & PF_WQ_WORKER))
+               return;
+
+       clear PF_WQ_WORKER, probably do more cleanups
+}
+
 /**
  * worker_thread - the worker thread function
  * @__worker: self
@@ -2171,11 +2179,14 @@ static void process_scheduled_works(stru
  */
 static int worker_thread(void *__worker)
 {
+       struct callback_head oops_work;
        struct worker *worker = __worker;
        struct worker_pool *pool = worker->pool;
 
        /* tell the scheduler that this is a workqueue worker */
        worker->task->flags |= PF_WQ_WORKER;
+       init_task_work(&oops_work, oops_handler);
+       task_work_add(current, &oops_work, false);
 woke_up:
        spin_lock_irq(&pool->lock);