linux-kernel - Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove

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: <1237926131.4320.127.camel@johannes.local>
Date:	Tue, 24 Mar 2009 21:22:11 +0100
From:	Johannes Berg <johannes@...solutions.net>
To:	Alex Chiang <achiang@...com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Oleg Nesterov <oleg@...hat.com>, jbarnes@...tuousgeek.org,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	kaneshige.kenji@...fujitsu.com,
	Lai Jiangshan <laijs@...fujitsu.com>
Subject: Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove

On Tue, 2009-03-24 at 11:23 -0600, Alex Chiang wrote:

> > There is no bug -- it's a false positive in a way. I've pointed this out
> > in the original thread, see
> > http://thread.gmane.org/gmane.linux.kernel/550877/focus=550932
> 
> I'm actually a bit confused now.

Sorry.

> Peter explained why flushing a workqueue from the same queue is
> bad, and in general I agree, but what do you mean by "false
> positive"?

Well, even though generally flushing it from within is bad, the actual
thing lockdep reports is bogus -- it's reporting a nested locking.

> By the way, this scenario:
> 
> 	code path 1:
> 	  my_function() -> lock(L1); ...; flush_workqueue(); ...
> 
> 	code path 2:
> 	  run_workqueue() -> my_work() -> ...; lock(L1); ...
> 
> is _not_ what is happening here.

Indeed.

> So what you really have going on is:
> 
> 	sysfs callback -> add remove callback to global workqueue
> 	remove callback fires off (pci_remove_bus_device) and we do...
> 	    device_unregister
> 	    driver's ->remove method called
> 	    driver's ->remove method calls flush_scheduled_work
> 
> Yes, after read the thread I agree that generically calling
> flush_workqueue in the middle of run_workqueue is bad, but the
> lockdep warning that Kenji showed us really won't deadlock.

Exactly that is what I meant by "false positive".

> This is because pci_remove_bus_device() will not acquire any lock
> L1 that an individual device driver will attempt to acquire in
> the remove path. If that were the case, we would deadlock every
> time you rmmod'ed a device driver's module or every time you shut
> your machine down.
> 
> I think from my end, there are 2 things I need to do:
> 
> 	a) make sysfs_schedule_callback() use its own work queue
> 	   instead of global work queue, because too many drivers
> 	   call flush_scheduled_work in their remove path
> 
> 	b) give sysfs attributes the ability to commit suicide
> 
> (a) is short term work, 2.6.30 timeframe, since it doesn't
> involve any large conceptual changes.
> 
> (b) is picking up Tejun Heo's existing work, but that was a bit
> controversial last time, and I'm not sure it will make it during
> this merge window.
> 
> Question for the lockdep folks though -- given what I described,
> do you agree that the warning we saw was a false positive? Or am
> I off in left field?

I think we're not sure yet -- it seems Lai Jiangshan described a
scenario in which flushing from within the work actually _can_ deadlock.

johannes

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)