Re: [PATCH net -v2] [BUGFIX] bonding: use flush_delayed_work_sync in bond_close

[Jay Vosburgh]

Jay Vosburgh <fubar@xxxxxxxxxx> wrote:

>AmÃrico Wang <xiyou.wangcong@xxxxxxxxx> wrote:
>
>>On Thu, Oct 20, 2011 at 3:09 AM, Jay Vosburgh <fubar@xxxxxxxxxx> wrote:
>>> Stephen Hemminger <shemminger@xxxxxxxxxx> wrote:
>>>
>>>>On Wed, 19 Oct 2011 11:01:02 -0700
>>>>Jay Vosburgh <fubar@xxxxxxxxxx> wrote:
>>>>
>>>>> Mitsuo Hayasaka <mitsuo.hayasaka.hu@xxxxxxxxxxx> wrote:
>>>>>
>>>>> >The bond_close() calls cancel_delayed_work() to cancel delayed works.
>>>>> >It, however, cannot cancel works that were already queued in workqueue.
>>>>> >The bond_open() initializes work->data, and proccess_one_work() refers
>>>>> >get_work_cwq(work)->wq->flags. The get_work_cwq() returns NULL when
>>>>> >work->data has been initialized. Thus, a panic occurs.
>>>>> >
>>>>> >This patch uses flush_delayed_work_sync() instead of cancel_delayed_work()
>>>>> >in bond_close(). It cancels delayed timer and waits for work to finish
>>>>> >execution. So, it can avoid the null pointer dereference due to the
>>>>> >parallel executions of proccess_one_work() and initializing proccess
>>>>> >of bond_open().
>>>>>
>>>>> Â Â ÂI'm setting up to test this. ÂI have a dim recollection that we
>>>>> tried this some years ago, and there was a different deadlock that
>>>>> manifested through the flush path. ÂPerhaps changes since then have
>>>>> removed that problem.
>>>>>
>>>>> Â Â Â-J
>>>>
>>>>Won't this deadlock on RTNL. ÂThe problem is that:
>>>>
>>>> Â CPU0 Â Â Â Â Â Â Â Â Â Â Â Â Â ÂCPU1
>>>> Ârtnl_lock
>>>> Â Â Âbond_close
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â delayed_work
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â mii_work
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â read_lock(bond->lock);
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â read_unlock(bond->lock);
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â rtnl_lock... waiting for CPU0
>>>> Â Â Âflush_delayed_work_sync
>>>> Â Â Â Â Âwaiting for delayed_work to finish...
>>>
>>> Â Â Â ÂYah, that was it. ÂWe discussed this a couple of years ago in
>>> regards to a similar patch:
>>>
>>> http://lists.openwall.net/netdev/2009/12/17/3
>>>
>>> Â Â Â ÂThe short version is that we could rework the rtnl_lock inside
>>> the montiors to be conditional and retry on failure (where "retry" means
>>> "reschedule the work and try again later," not "spin retrying on rtnl").
>>> That should permit the use of flush or cancel to terminate the work
>>> items.
>>
>>Yes? Even if we use rtnl_trylock(), doesn't flush_delayed_work_sync()
>>still queue the pending delayed work and wait for it to be finished?
>
>	Yes, it does.  The original patch wants to use flush instead of
>cancel to wait for the work to finish, because there's evidently a
>possibility of getting back into bond_open before the work item
>executes, and bond_open would reinitialize the work queue and corrupt
>the queued work item.
>
>	The original patch series, and recipe for destruction, is here:
>
>	http://www.spinics.net/lists/netdev/msg176382.html
>
>	I've been unable to reproduce the work queue panic locally,
>although it sounds plausible.
>
>	Mitsuo: can you provide the precise bonding configuration you're
>using to induce the problem?  Driver options, number and type of slaves,
>etc.
>
>>Maybe I am too blind, why do we need rtnl_lock for cancel_delayed_work()
>>inside bond_close()?
>
>	We don't need RTNL for cancel/flush.  However, bond_close is an
>ndo_stop operation, and is called in the dev_close path, which always
>occurs under RTNL.  The mii / arp monitor work functions separately
>acquire RTNL if they need to perform various failover related
>operations.
>
>	I'm working on a patch that should resolve the mii / arp monitor
>RTNL problem as I described above (if rtnl_trylock fails, punt and
>reschedule the work).  I need to rearrange the netdev_bonding_change
>stuff a bit as well, since it acquires RTNL separately.
>
>	Once these changes are made to mii / arp monitor, then
>bond_close can call flush instead of cancel, which should eliminate the
>original problem described at the top.

	Just an update: there are three functions that may deadlock if
the cancel work calls are changed to flush_sync.  There are two
rtnl_lock calls in each of the bond_mii_monitor and
bond_activebackup_arp_mon functions, and one more in the
bond_alb_monitor.

	Still testing to make sure I haven't missed anything, and I
still haven't been able to reproduce Mitsuo's original failure.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@xxxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/