linux-kernel - Re: [PATCH 1/2] workqueue: rescuer_thread() processes all pwqs before exit

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: <CACvQF527RneJwUXf0=C6L0_9ncdS6--iqhdpWAFKqHtd25LOYg@mail.gmail.com>
Date:	Fri, 18 Apr 2014 00:04:29 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] workqueue: rescuer_thread() processes all pwqs before exit

On Thu, Apr 17, 2014 at 11:27 PM, Tejun Heo <tj@...nel.org> wrote:
> Hello,
>
> On Thu, Apr 17, 2014 at 07:34:08AM +0800, Lai Jiangshan wrote:
>> Before the rescuer is picked to running, the works of the @pwq
>> may be processed by some other workers, and destroy_workqueue()
>> may called at the same time. This may result a nasty situation
>> that rescuer may exit with non-empty mayday list.
>>
>> It is no harm currently, destroy_workqueue() can safely to free
>> them all(workqueue&pwqs) togerther, since the rescuer is stopped.
>> No rescuer nor mayday-timer can access the mayday list.
>>
>> But it is nasty and error-prone in future develop. Fix it.
>>
>> Signed-off-by: Lai Jiangshan <laijs@...fujitsu.com>
>> ---
>>  kernel/workqueue.c |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 0ee63af..832125f 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -2409,12 +2409,6 @@ static int rescuer_thread(void *__rescuer)
>>  repeat:
>>       set_current_state(TASK_INTERRUPTIBLE);
>>
>> -     if (kthread_should_stop()) {
>> -             __set_current_state(TASK_RUNNING);
>> -             rescuer->task->flags &= ~PF_WQ_WORKER;
>> -             return 0;
>> -     }
>> -
>>       /* see whether any pwq is asking for help */
>>       spin_lock_irq(&wq_mayday_lock);
>>
>> @@ -2459,6 +2453,12 @@ repeat:
>>
>>       spin_unlock_irq(&wq_mayday_lock);
>>
>> +     if (kthread_should_stop()) {
>> +             __set_current_state(TASK_RUNNING);
>> +             rescuer->task->flags &= ~PF_WQ_WORKER;
>> +             return 0;
>> +     }
>> +
>
> I don't think this is reliable.  What if mayday requests take place
> between wq_mayday_lock and kthread_should_stop() check?  We'll
> probably need to run through mayday list after checking should_stop.

It is destroy_workqueue()'s responsibility to avoid this.
destroy_workqueue() should drain all works and refuse any new work queued
on the wq before destroy the wq.

So since there is no works, there is no new mayday request,
and there is no mayday request take place between wq_mayday_lock
and kthread_should_stop() check.

Thanks,
Lai

>
> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/