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
[-- Attachment #1 --]
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
> >
[-- Attachment #2 --]
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140204062731.2e9d4bc0>