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

[Roman Penyaev]

On Fri, Oct 21, 2016 at 5:47 PM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> 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.

I will send a patch if reproduce it :)

>
>> >                          */
>> > -                       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().

Yeah, probably handling this corner case in wq_worker_sleeping() func
is much better.

>
> Or perhaps something like the change below.

That's a nice stuff, thanks Oleg. I simply did not know about these
callbacks.

But the huge problem is that after commit 2deb4be28 by Andy Lutomirski
we can't use stack when we are already in do_exit().  And putting this
callback head inside a worker structure is a bloat.  I will resend this
with a simple task state check in a wq_worker_sleeping().

--
Roman


>
> 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);
>
>