linux-kernel - Re: [patch, minor] workqueue: consistently use 'err' in __create_workqueue

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: <b647ffbd0807290458n23dbe79et45cb27273e4fc2ae@mail.gmail.com>
Date:	Tue, 29 Jul 2008 13:58:02 +0200
From:	"Dmitry Adamushko" <dmitry.adamushko@...il.com>
To:	"Oleg Nesterov" <oleg@...sign.ru>
Cc:	linux-kernel@...r.kernel.org, "Ingo Molnar" <mingo@...e.hu>
Subject: Re: [patch, minor] workqueue: consistently use 'err' in __create_workqueue_key()

2008/7/29 Oleg Nesterov <oleg@...sign.ru>:
> On 07/28, Dmitry Adamushko wrote:
>>
>> I guess error handling is a bit illogical in __create_workqueue_key()
>
> Please see below,
>
>>               for_each_possible_cpu(cpu) {
>>                       cwq = init_cpu_workqueue(wq, cpu);
>> -                     if (err || !cpu_online(cpu))
>> +                     if (!cpu_online(cpu))
>>                               continue;
>>                       err = create_workqueue_thread(cwq, cpu);
>> +                     if (err)
>> +                             break;
>
> This was done on purpose. The code above does init_cpu_workqueue(cpu)
> for each possible cpu, even if we fail to create cwq->thread for some
> cpu. This way destroy_workqueue() (called below) shouldn't worry about
> the partially initialized workqueues.
>
> The patch above should work, but it assumes that destroy_workqueue()
> must do nothing with cwq if cwq->thread == NULL, this is not very
> robust.

Yes, I saw this test and that's why I decided that destroy_workqueue()
is able (designed) to deal with partially-initialized objects.

Note, for the race scenario with cpu-hotplug (which I've overlooked
indeed) which you describe below, we also seem to depend on the same
"cwq->thread == NULL" test in cleanup_workqueue_thread() as follows:

assume, cpu_down(cpu) -> CPU_POST_DEAD -> cleanup_workqueue_thread()
gets called for a partially initialized workqueue for 'cpu' for which
create_workqueue_thread() has previously failed in
create_worqueue_key().

>
> And, more importantly. Let's suppose __create_workqueue_key() does
> "break" and drops cpu_add_remove_lock. Then we race with cpu-hotplug
> which can hit the uninitialized cwq. This is fixable, but needs other
> complication.

And I'd say this behavior (of having a partially-created object
visible to the outside world) is not that robust. e.g. the
aforementioned race would be eliminated if we place a wq on the global
list only when it's been successfully initialized.

For this goal, the cleanup path in __create_workqueue_key() would need
to be altered but overall, I think it'd make the code a bit more
straightforward.

[ just my 0.02, maybe I'm missing something again ;-) ]



>
> Oleg.
>


-- 
Best regards,
Dmitry Adamushko
--
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/