netdev - Re: [Bonding-devel] [PATCH] bonding: cancel_delayed_work() -> cancel_delayed_work

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]

Date:	Thu, 17 Dec 2009 21:56:17 +0100
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Jay Vosburgh <fubar@...ibm.com>
Cc:	Laurent Chavey <chavey@...gle.com>,
	Johannes Berg <johannes@...solutions.net>,
	Mikhail Markine <markine@...gle.com>, netdev@...r.kernel.org,
	bonding-devel@...ts.sourceforge.net,
	Petri Gynther <pgynther@...gle.com>,
	David Miller <davem@...emloft.net>
Subject: Re: [Bonding-devel] [PATCH] bonding: cancel_delayed_work() ->
 cancel_delayed_work_sync()

On Thu, Dec 17, 2009 at 11:37:42AM -0800, Jay Vosburgh wrote:
> Laurent Chavey <chavey@...gle.com> wrote:
> 
> >one instance that could be a problem
> >
> >__exit bonding_exit(void)
> >    bond_free_all()
> >      	bond_work_cancel_all(bond);
> >        unregister_netdevice(bond_dev)
> >
> >could the above result in an invalid pointer when trying
> >to use bond-> in one of the timer CB ?
> 
> 	The bonding teardown logic was reworked in October, and there is
> no longer a bond_free_all in the current mainline.  What kernel are you
> looking at?
> 
> 	The bond_close function will stop the various work items, and
> the ndo_uninit (bond_uninit) will call bond_work_cancel_all as well.
> 
> 	Actually, on looking at it (it being current mainline),
> bond_uninit might need some kind of logic to wait and insure that all
> timers have completed before returning.  It comes from unregister, so
> the next thing that happens after it returns is that the memory will be
> freed (via netdev_run_todo, during rtnl_unlock, if I'm following it
> correctly).
> 
> 	The bond_uninit function is called under RTNL, though, so the
> timer functions (bond_mii_monitor, et al) may need additional checks for
> kill_timers to insure they don't attempt to acquire RTNL if a cancel is
> pending.
> 
> 	That's kind of tricky itself, since the lock ordering requires
> RTNL to be acquired first, so there's no way for bond_mii_monitor (et
> al) to check for kill_timers prior to already having RTNL (because the
> function acquires RTNL conditionally, only if needed; to do that, it
> unlocks the bond lock, then acquires RTNL, then re-locks the bond lock).
> 
> 	So, the lock dance to acquire RTNL in bond_mii_monitor (et al)
> would need some trickery, perhaps a rtnl_trylock loop, that checks
> kill_timers each time the trylock fails, e.g.,
> 
> 	if (bond_miimon_inspect(bond)) {
> 		read_unlock(&bond->lock);
> 		while (!rtnl_trylock) {
> 			read_lock(&bond->lock);
> 			if (bond->kill_timers)
> 				goto out;
> 			read_unlock(&bond->lock);
> 			/* msleep ? */
> 		}
> 
> 		bond_miimon_commit(bond);
> 		[...]
> 
> 	So, with the above (and similar changes to the other delayed
> work functions, and a big honkin' comment somewhere to explain this), I
> suspect that bond_work_cancel_all could use the _sync variant to cancel
> the work, as long as kill_timers is set before the cancel_sync is
> called.
> 
> 	Am I missing anything?  Does this seem rational?

It seems OK to me ...if there is nothing better ;-) But such endless
loops are tricky (they omit lockdep, plus can hide some hidden
dependancies between different tasks, even in the future). If it's
possible we could consider a limited loop with re-arming on failure;
then cancel_delayed_work_sync() (with its standard logic) could be
used everywhere, and kill_timers might be useless too (if there is no
re-arming between different works).

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html