linux-kernel - Re: [PATCH] cpusets: fix deadlock with cpu_down()->cpuset

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: <4A725594.8020205@cn.fujitsu.com>
Date:	Fri, 31 Jul 2009 10:23:16 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Rusty Russell <rusty@...tcorp.com.au>,
	linux-kernel@...r.kernel.org, Li Zefan <lizf@...fujitsu.com>,
	Miao Xie <miaox@...fujitsu.com>,
	Paul Menage <menage@...gle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Gautham R Shenoy <ego@...ibm.com>
Subject: Re: [PATCH] cpusets: fix deadlock with cpu_down()->cpuset_lock()

Oleg Nesterov wrote:
> On 07/30, Lai Jiangshan wrote:
>> Oleg Nesterov wrote:
>>> On 07/29, Oleg Nesterov wrote:
>>>> I strongly believe the bug does exist, but this patch needs the review
>>>> from maintainers.
>>> Yes...
>>>
>>>> IOW, with this patch migration_call(CPU_DEAD) runs without callback_mutex,
>>>> but kernel/cpuset.c always takes get_online_cpus() before callback_mutex.
>>> Oh. I'm afraid this is not an option.
>>>
>>> callback_mutex should nest under cgroup_mutex, but cpu hotplu pathes
>>> take cgroup_mutex under cpu_hotplug->lock. Lockdep won't be happy.
>>>
>>> Oleg.
>>>
>> We have made great effort to remove get_online_cpus() from cgroup_mutex
>> critical region.
> 
> Agreed.
> 
>> We can migrate the owner of callback_mutex in migration_call(CPU_DEAD)
>> at first(and then take callback_mutex and migrate others).
> 
> Not sure I understand how can we do this. Even if we know the owner
> of callback_mutex, if we can migrate it safely without callback_mutex
> why we can't migrate other tasks without this lock?

Since we have migrated the owner, we can take callback_mutex to
migrate others ... 

> 
> In any case this doesn't look like a clean solution,

No, it's not a clean solution.

> imho. But I hardly understand what cpuset is,


> can't suggest something clever.

We can add cpuset_lock()/cpuset_unlock() around __stop_machine()
in _cpu_down().

cpuset_lock()
__stop_machine()
	......
	mutex_lock(&lock);
	# It's OK, because we don't require any other lock in this
	# critical region. It's will not cause any kinds of deadlock.
	......
	flush_workqueue(stop_machine_wq);
	# It's OK too. because all work functions(chill(),stop_cpu())
	# of stop_machine_wq don't require any other lock.
	......
	mutex_unlock(&lock);
cpuset_unlock()


This fixes the bug in migrate_call(). Because there is no task which
holds callback_mutex in dead cpu after we add
cpuset_lock()/cpuset_unlock() around __stop_machine() in _cpu_down().

And it helps for your "cpu_hotplug: don't play with current->cpus_allowed"
Am I right?

Lai

--
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/