Ordering problem in if_detach_internal regarding if_bridge

[John Baldwin]

On Friday, June 20, 2014 11:25:51 am Roger Pau Monné wrote:
> Hello,
> 
> I've stumbled across the following panic when testing Xen netback with 
> if_bridge:
> 
> Kernel page fault with the following non-sleepable locks held:
> exclusive sleep mutex if_bridge (if_bridge) r = 0 (0xfffff80006306c18) 
locked @ /usr/src/sys/m
> KDB: stack backtrace:
> X_db_symbol_values() at X_db_symbol_values+0x10b/frame 0xfffffe0000213490
> kdb_backtrace() at kdb_backtrace+0x39/frame 0xfffffe0000213540
> witness_warn() at witness_warn+0x4a8/frame 0xfffffe0000213600
> trap() at trap+0xc9d/frame 0xfffffe00002136a0
> trap() at trap+0x669/frame 0xfffffe00002138b0
> calltrap() at calltrap+0x8/frame 0xfffffe00002138b0
> --- trap 0xc, rip = 0xffffffff8221a0ef, rsp = 0xfffffe0000213970, rbp = 
0xfffffe00002139e0 ---
> bridge_input() at bridge_input+0x5ff/frame 0xfffffe00002139e0
> ether_vlanencap() at ether_vlanencap+0x4a3/frame 0xfffffe0000213a10
> netisr_dispatch_src() at netisr_dispatch_src+0x90/frame 0xfffffe0000213a80
> ether_ifattach() at ether_ifattach+0x19f/frame 0xfffffe0000213ab0
> ath_dfs_get_thresholds() at ath_dfs_get_thresholds+0x81ce/frame 
0xfffffe0000213b30
> intr_event_execute_handlers() at intr_event_execute_handlers+0x93/frame 
0xfffffe0000213b70
> db_dump_intr_event() at db_dump_intr_event+0x796/frame 0xfffffe0000213bb0
> fork_exit() at fork_exit+0x84/frame 0xfffffe0000213bf0
> fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe0000213bf0
> --- trap 0, rip = 0, rsp = 0xfffffe0000213cb0, rbp = 0 ---
> 
> I've tracked this down to if_detach_internal setting ifp->if_addr to 
> NULL before calling EVENTHANDLER_INVOKE(ifnet_departure_event..., which 
> causes a panic in GRAB_OUR_PACKETS in the if_bridge code when it tries 
> to perform IF_LLADDR on an interface that's in the process of being 
> destroyed (ifp->if_addr set to NULL, but the ifnet_departure_event event 
> has not fired yet).
> 
> I have the following naive patch that moves the firing of the event 
> before if_addr is set to NULL, but I'm not familiar with the ordering 
> in if_detach_internal, so I'm not sure if this might cause problems in 
> other parts of the code, could someone familiar with the net stuff 
> comment on the best way to deal with it?

Hmmm, I have no idea if this is ok or not.  I do think the route message 
should go out at the same time as the devctl_notify() call however.  My guess 
is it is actually better to do this earlier so that we allow outside consumers
to detach from an interface before it is destroyed.  I'm not sure if it would
break things, but I would be tempted to move this even earlier right after it
is removed from the global ifnet list but before the taskqueue_drain, etc.

-- 
John Baldwin