linux-kernel - Re: [PATCH 01/10] sysfs: don't use global workqueue in sysfs_schedule

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] [day] [month] [year] [list]
Date:	Mon, 27 Apr 2009 23:15:49 -0600
From:	Alex Chiang <achiang@...com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>, linux-kernel@...r.kernel.org,
	Lai Jiangshan <laijs@...fujitsu.com>, a.p.zijlstra@...llo.nl
Subject: Re: [PATCH 01/10] sysfs: don't use global workqueue in
	sysfs_schedule_callback()

* Andrew Morton <akpm@...ux-foundation.org>:
> On Fri, 17 Apr 2009 12:04:25 -0700
> Greg Kroah-Hartman <gregkh@...e.de> wrote:
> 
> > From: Alex Chiang <achiang@...com>
> > 
> > A sysfs attribute using sysfs_schedule_callback() to commit suicide
> > may end up calling device_unregister(), which will eventually call
> > a driver's ->remove function.
> > 
> > Drivers may call flush_scheduled_work() in their shutdown routines,
> > in which case lockdep will complain with something like the following:
> > 
> >   =============================================
> >   [ INFO: possible recursive locking detected ]
> >   2.6.29-rc8-kk #1
> >   ---------------------------------------------
> >   events/4/56 is trying to acquire lock:
> >   (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0
> > 
> >   but task is already holding lock:
> >   (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
> > 
> >   other info that might help us debug this:
> >   3 locks held by events/4/56:
> >   #0:  (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
> >   #1:  (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
> >   #2:  (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40
> > 
> >   stack backtrace:
> >   Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1
> >   Call Trace:
> >   [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260
> >   [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40
> >   [<ffffffff8026f148>] lock_acquire+0x58/0x80
> >   [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
> >   [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0
> >   [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
> >   [<ffffffff80258070>] flush_scheduled_work+0x10/0x20
> >   [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e]
> >   [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50
> >   [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70
> >   [<ffffffff80441da9>] __device_release_driver+0x59/0x90
> >   [<ffffffff80441edb>] device_release_driver+0x2b/0x40
> >   [<ffffffff804419d6>] bus_remove_device+0xa6/0x120
> >   [<ffffffff8043e46b>] device_del+0x12b/0x190
> >   [<ffffffff8043e4f6>] device_unregister+0x26/0x70
> >   [<ffffffff803ba969>] pci_stop_dev+0x49/0x60
> >   [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0
> >   [<ffffffff803c10d9>] remove_callback+0x29/0x40
> >   [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50
> >   [<ffffffff8025769a>] run_workqueue+0x15a/0x230
> >   [<ffffffff80257648>] ? run_workqueue+0x108/0x230
> >   [<ffffffff8025846f>] worker_thread+0x9f/0x100
> >   [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40
> >   [<ffffffff802583d0>] ? worker_thread+0x0/0x100
> >   [<ffffffff8025b89d>] kthread+0x4d/0x80
> >   [<ffffffff8020d4ba>] child_rip+0xa/0x20
> >   [<ffffffff8020cebc>] ? restore_args+0x0/0x30
> >   [<ffffffff8025b850>] ? kthread+0x0/0x80
> >   [<ffffffff8020d4b0>] ? child_rip+0x0/0x20
> > 
> > Although we know that the device_unregister path will never acquire
> > a lock that a driver might try to acquire in its ->remove, in general
> > we should never attempt to flush a workqueue from within the same
> > workqueue, and lockdep rightly complains.
> > 
> > So as long as sysfs attributes cannot commit suicide directly and we
> > are stuck with this callback mechanism, put the sysfs callbacks on
> > their own workqueue instead of the global one.
> > 
> > This has the side benefit that if a suicidal sysfs attribute kicks
> > off a long chain of ->remove callbacks, we no longer induce a long
> > delay on the global queue.
> 
> I still don't know why I merged 
> 
> : commit 2355b70fd59cb5be7de2052a9edeee7afb7ff099
> : Author: Lai Jiangshan <laijs@...fujitsu.com>
> : Date:   Thu Apr 2 16:58:24 2009 -0700
> :
> :    workqueue: avoid recursion in run_workqueue()
> 
> there was nothing wrong with permitting limited recursion into
> run_workqueue().  It never deadlocked and the three-deep-recursion
> warning never triggered.

According to Peter Zjilstra and Lai Jiangshan, there actually is
danger of deadlock when flushing a workqueue from the same
workqueue:

	http://thread.gmane.org/gmane.linux.kernel.pci/3713/focus=811703

And in any case, I think it's a good idea to avoid the global
workqueue, as removing a PCI bridge near the root of the
hierarchy may result in a longish series of operations as we
unregister drivers, etc.

> > +	if (sysfs_workqueue == NULL) {
> > +		sysfs_workqueue = create_workqueue("sysfsd");
> > +		if (sysfs_workqueue == NULL) {
> > +			module_put(owner);
> > +			return -ENOMEM;
> > +		}
> > +	}
> 
> This will create a kernel thread per CPU.  Surely
> create_singlethread_workqueue() will suffice?

Oh darn, you are right. We do not need a per-CPU thread. I will
queue up a patch to use create_singlethread_workqueue.

Thanks for the review.

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