Date: Mon, 23 Jun 2014 20:49:17 +0400 From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: John Baldwin <jhb@freebsd.org>, freebsd-net@freebsd.org Cc: =?ISO-8859-15?Q?Roger_Pau_Monn=E9?= <roger.pau@citrix.com> Subject: Re: Ordering problem in if_detach_internal regarding if_bridge Message-ID: <53A85A8D.4090208@FreeBSD.org> In-Reply-To: <53A8585C.3080000@FreeBSD.org> References: <53A4527F.90008@citrix.com> <201406231132.47487.jhb@freebsd.org> <53A8585C.3080000@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 23.06.2014 20:39, Alexander V. Chernikov wrote: > On 23.06.2014 19:32, John Baldwin wrote: >> 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? > > We should notify kernel customers only when we are really taking this > interface down and every other subsystem cannot add any new state to the > interface. > > In this patch you're sending notification before taking ifnet down, > removing its L3 addresses, routes, and so on. > > This can easily lead to panic in, for example, BPF subsystem (since BPF > state is freed in bpf_ifdetach() handler). > > Addintionally, this will introduce ifaddr / iface messages reversal for > rtsock. Whoops. I misread the patch. It should be OK. > > It looks like we'd better fix if_bridge (and it is still using mutexes, > what a shame!). > > Can you send me trace with line numbers? However, these two still stands. (And I'm wondering how you're getting any traffic on down/dying interface). > >> >> 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. >> > > _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53A85A8D.4090208>