Date: Tue, 4 Feb 2014 06:27:31 +0100 From: Marko Zec <zec@fer.hr> To: Vijay Singh <vijju.singh@gmail.com> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org> Subject: Re: vnet deletion panic Message-ID: <20140204062731.2e9d4bc0@x23.lan> In-Reply-To: <CALCNsJTsCesABH=Zm5ud4-SktwZg1pNt3cwCC6uu=Z0znspBug@mail.gmail.com> References: <CALCNsJQSfqyXUuiGUPwmuXH3OCdmMRVSZtZSDQEBTb9csQAe4Q@mail.gmail.com> <20140204055229.4a52ec15@x23.lan> <CALCNsJTsCesABH=Zm5ud4-SktwZg1pNt3cwCC6uu=Z0znspBug@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--MP_/.h=lNEG+_Pdgcpx_45mRJX2 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Content-Disposition: inline On Mon, 3 Feb 2014 21:05:28 -0800 Vijay Singh <vijju.singh@gmail.com> wrote: > Hi Marco, the code in rt_ifmsg() checks what seems like global state, > so its not routing sockets in the vnet being destroyed. Huh then we should V_irtualize that global state - maybe the attached patch could help (only compile-tested, perhaps you could try it out)? Marko > rt_ifmsg(struct ifnet *ifp) > { > struct if_msghdr *ifm; > struct mbuf *m; > struct rt_addrinfo info; > > if (route_cb.any_count == 0) > return; > > You are right, there is no ifp context in rt_dispatch(). So perhaps we > should not call rt_ifmsg() from if_unroute() is (ifp == V_loif) since > that would end up using the soon to be destroyed ifp in the mbuf. > What do you think? > > > On Mon, Feb 3, 2014 at 8:52 PM, Marko Zec <zec@fer.hr> wrote: > > > On Mon, 3 Feb 2014 19:33:21 -0800 > > Vijay Singh <vijju.singh@gmail.com> wrote: > > > > > I'm running into a crash due on vnet deletion in the presence of > > > routing sockets. The root cause seems to originate from(): > > > > > > if_detach_internal() -> if_down(ifp) -> if_unroute() -> > > > rt_ifmsg() -> rt_dispatch() > > > > > > In rt_dispatch() we have: > > > > > > #ifdef VIMAGE > > > if (V_loif) > > > m->m_pkthdr.rcvif = V_loif; > > > #endif > > > netisr_queue(NETISR_ROUTE, m); > > > > > > Now since this would be processed async, and the ifp alove is the > > > loopback of the vnet being deleted, we run into accessing a freed > > > pointer (ifp) when netisr picks up the mbuf. So I am wondering > > > how to fix this. I am thinking that we could do something like > > > the following in rt_dispatch(): > > > > > > #ifdef VIMAGE > > > if (V_loif) { > > > if ((ifp == V_loif) && !IS_DEFAULT_VNET(curvnet)) { > > > CURVNET_SET_QUIET(vnet0); > > > m->m_pkthdr.rcvif = V_loif; > > > CURVNET_RESTORE(); > > > } else > > > m->m_pkthdr.rcvif = V_loif; > > > } > > > #endif > > > > > > So basically switch to the default vnet for the mbuf with the > > > routing socket message. Thoughts? > > > > By design, the vnet teardown procedure should not commence before > > the last socket attached to that vnet is closed, so I'm suspicious > > whether the proposed approach could actually appease the panics > > you're observing. Furthermore, it would certainly cause bogus > > routing messages to appear in vnet0 and possibly confuse routing > > socket consumers running there. Plus, in rt_dispatch() there's no > > ifp context to check against V_loif at all, as you're proposing > > your patch? > > > > Perhaps it could be possible to walk through all the netisr queues > > just before V_loif gets destroyed, and prune all queued mbufs which > > have m->m_pkthdr.rcvif pointing to V_loif? Since the vnet teardown > > procedure cannot be initiated before all (routing) sockets attached > > to that vnet have been closed, after all other ifnets except V_loif > > have also been destroyed it should not be possible for new mbufs to > > be queued with rcvif pointing back to V_loif, so at least > > conceptually that approach might work correctly. > > > > Marko > > --MP_/.h=lNEG+_Pdgcpx_45mRJX2 Content-Type: text/x-patch Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v_route_cb.diff" Index: rtsock.c =================================================================== --- rtsock.c (revision 258393) +++ rtsock.c (working copy) @@ -156,12 +156,14 @@ #define RTS_FILTER_FIB M_PROTO8 #define RTS_ALLFIBS -1 -static struct { +typedef struct { int ip_count; /* attached w/ AF_INET */ int ip6_count; /* attached w/ AF_INET6 */ int ipx_count; /* attached w/ AF_IPX */ int any_count; /* total attached */ -} route_cb; +} route_cb_t; +static VNET_DEFINE(route_cb_t, route_cb); +#define V_route_cb VNET(route_cb) struct mtx rtsock_mtx; MTX_SYSINIT(rtsock, &rtsock_mtx, "rtsock route_cb lock", MTX_DEF); @@ -328,16 +330,16 @@ RTSOCK_LOCK(); switch(rp->rcb_proto.sp_protocol) { case AF_INET: - route_cb.ip_count++; + V_route_cb.ip_count++; break; case AF_INET6: - route_cb.ip6_count++; + V_route_cb.ip6_count++; break; case AF_IPX: - route_cb.ipx_count++; + V_route_cb.ipx_count++; break; } - route_cb.any_count++; + V_route_cb.any_count++; RTSOCK_UNLOCK(); soisconnected(so); so->so_options |= SO_USELOOPBACK; @@ -372,16 +374,16 @@ RTSOCK_LOCK(); switch(rp->rcb_proto.sp_protocol) { case AF_INET: - route_cb.ip_count--; + V_route_cb.ip_count--; break; case AF_INET6: - route_cb.ip6_count--; + V_route_cb.ip6_count--; break; case AF_IPX: - route_cb.ipx_count--; + V_route_cb.ipx_count--; break; } - route_cb.any_count--; + V_route_cb.any_count--; RTSOCK_UNLOCK(); raw_usrreqs.pru_detach(so); } @@ -928,7 +930,7 @@ * Check to see if we don't want our own messages. */ if ((so->so_options & SO_USELOOPBACK) == 0) { - if (route_cb.any_count <= 1) { + if (V_route_cb.any_count <= 1) { if (rtm) Free(rtm); m_freem(m); @@ -1217,7 +1219,7 @@ struct mbuf *m; struct sockaddr *sa = rtinfo->rti_info[RTAX_DST]; - if (route_cb.any_count == 0) + if (V_route_cb.any_count == 0) return; m = rt_msg1(type, rtinfo); if (m == NULL) @@ -1255,7 +1257,7 @@ struct mbuf *m; struct rt_addrinfo info; - if (route_cb.any_count == 0) + if (V_route_cb.any_count == 0) return; bzero((caddr_t)&info, sizeof(info)); m = rt_msg1(RTM_IFINFO, &info); @@ -1299,7 +1301,7 @@ sctp_addr_change(ifa, cmd); #endif /* SCTP */ #endif - if (route_cb.any_count == 0) + if (V_route_cb.any_count == 0) return; for (pass = 1; pass < 3; pass++) { bzero((caddr_t)&info, sizeof(info)); @@ -1368,7 +1370,7 @@ struct ifnet *ifp = ifma->ifma_ifp; struct ifma_msghdr *ifmam; - if (route_cb.any_count == 0) + if (V_route_cb.any_count == 0) return; bzero((caddr_t)&info, sizeof(info)); @@ -1397,7 +1399,7 @@ struct if_announcemsghdr *ifan; struct mbuf *m; - if (route_cb.any_count == 0) + if (V_route_cb.any_count == 0) return NULL; bzero((caddr_t)info, sizeof(*info)); m = rt_msg1(type, info); --MP_/.h=lNEG+_Pdgcpx_45mRJX2--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140204062731.2e9d4bc0>