From owner-p4-projects@FreeBSD.ORG Mon May 3 22:18:38 2010 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 1E6C81065672; Mon, 3 May 2010 22:18:38 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id D6F6E106566C for ; Mon, 3 May 2010 22:18:37 +0000 (UTC) (envelope-from zec@icir.org) Received: from munja.zvne.fer.hr (munja.zvne.fer.hr [161.53.66.248]) by mx1.freebsd.org (Postfix) with ESMTP id 3BA7C8FC0A for ; Mon, 3 May 2010 22:18:36 +0000 (UTC) Received: from sluga.fer.hr ([161.53.66.244]) by munja.zvne.fer.hr with Microsoft SMTPSVC(6.0.3790.4675); Tue, 4 May 2010 00:06:29 +0200 Received: from localhost ([161.53.19.8]) by sluga.fer.hr over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Tue, 4 May 2010 00:06:28 +0200 From: Marko Zec To: Ana Kukec Date: Tue, 4 May 2010 00:06:10 +0200 User-Agent: KMail/1.9.10 References: <201005032103.o43L3nah081080@repoman.freebsd.org> In-Reply-To: <201005032103.o43L3nah081080@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <201005040006.11204.zec@icir.org> X-OriginalArrivalTime: 03 May 2010 22:06:29.0708 (UTC) FILETIME=[E1FD28C0:01CAEB0C] Cc: Perforce Change Reviews Subject: Re: PERFORCE change 177677 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 May 2010 22:18:38 -0000 On Monday 03 May 2010 23:03:49 Ana Kukec wrote: > http://p4web.freebsd.org/@@177677?ac=10 > > Change 177677 by anchie@anchie_malimis on 2010/05/03 21:03:44 > > Getting rid of the global variable V_send_so from files other then > send.[ch]. Just wondering - isn't this change actually increasing the possibility for a race between packet datapath and send.ko kldloading / kldunloading? I.e. had we kept the V_send_so variable global, we could (at least with more confidence) avoid calling into send hooks when they are in an intermitent state purely by checking whether V_send_so is NULL. Now we can't tell whether another thread is just in the process of kldunloading the send module while we are calling into one of the send hooks in our thread. Also, if I'm not mistaking, even if send_sendso_input() returns an error, it will have the mbuf consumed / freed, so any further attempts to do anything with the mbuf will crash the system. Do we really want to do m_freem(m) at the bottom of send_sendso_input()? Marko > Affected files ... > > .. //depot/projects/soc2009/anchie_send/src/sys/netinet6/icmp6.c#39 edit > .. //depot/projects/soc2009/anchie_send/src/sys/netinet6/nd6.c#29 edit > .. //depot/projects/soc2009/anchie_send/src/sys/netinet6/nd6_nbr.c#16 edit > .. //depot/projects/soc2009/anchie_send/src/sys/netinet6/raw_ip6.c#10 edit > .. //depot/projects/soc2009/anchie_send/src/sys/netinet6/send.c#43 edit > .. //depot/projects/soc2009/anchie_send/src/sys/netinet6/send.h#21 edit > > Differences ... > > ==== //depot/projects/soc2009/anchie_send/src/sys/netinet6/icmp6.c#39 > (text+ko) ==== > > @@ -424,7 +424,7 @@ > int icmp6len = m->m_pkthdr.len - *offp; > int code, sum, noff; > char ip6bufs[INET6_ADDRSTRLEN], ip6bufd[INET6_ADDRSTRLEN]; > - int ip6len; > + int ip6len, error = -1; > > ifp = m->m_pkthdr.rcvif; > > @@ -780,24 +780,37 @@ > /* give up local */ > > /* Send incoming SeND-protected/ND packet to user space. */ > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - IP6_EXTHDR_CHECK(m, off, icmp6len, IPPROTO_DONE); > - printf("send_sendso_input_hook\n"); > - send_sendso_input_hook(V_send_so, m, SND_IN, ip6len); > - } else { > + if (send_sendso_input_hook != NULL) { > + IP6_EXTHDR_CHECK(m, off, > + icmp6len, IPPROTO_DONE); > + error = send_sendso_input_hook(m, > + SND_IN, ip6len); > + /* -1 == no app on SEND socket */ > + if (!error) > + return (IPPROTO_DONE); > + } > + if ((send_sendso_input_hook != NULL > + && error == -1) || > + send_sendso_input_hook == NULL) { > /* give up local */ > nd6_rs_input(m, off, icmp6len); > } > m = NULL; > goto freeit; > } > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - IP6_EXTHDR_CHECK(m, off, icmp6len, IPPROTO_DONE); > - printf("send_sendso_input_hook\n"); > - send_sendso_input_hook(V_send_so, n, SND_IN, > ip6len); - return (IPPROTO_DONE); > - } else > + if (send_sendso_input_hook != NULL) { > + IP6_EXTHDR_CHECK(m, off, > + icmp6len, IPPROTO_DONE); > + error = send_sendso_input_hook(n, > + SND_IN, ip6len); > + /* -1 == no app on SEND socket */ > + if (!error) > + return (IPPROTO_DONE); > + } > + if ((send_sendso_input_hook != NULL && error == -1) > + || (send_sendso_input_hook == NULL)) { > nd6_rs_input(n, off, icmp6len); > + } > /* m stays. */ > break; > > @@ -810,20 +823,30 @@ > if ((n = m_copym(m, 0, M_COPYALL, M_DONTWAIT)) == NULL) { > > /* Send incoming SeND-protected/ND packet to user space. */ > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - > - send_sendso_input_hook(V_send_so, m, SND_IN, ip6len); > - return (IPPROTO_DONE); > - } else > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(m, > + SND_IN, ip6len); > + if (!error) > + return (IPPROTO_DONE); > + } > + if ((send_sendso_input_hook != NULL > + && error == -1) || > + send_sendso_input_hook == NULL) { > nd6_ra_input(m, off, icmp6len); > + } > m = NULL; > goto freeit; > } > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - send_sendso_input_hook(V_send_so, n, SND_IN, ip6len); > - return (IPPROTO_DONE); > - } else > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(n, > + SND_IN, ip6len); > + if (!error) > + return (IPPROTO_DONE); > + } > + if ((send_sendso_input_hook != NULL && error == -1) > + || (send_sendso_input_hook == NULL)) { > nd6_ra_input(n, off, icmp6len); > + } > /* m stays. */ > break; > > @@ -834,23 +857,27 @@ > if (icmp6len < sizeof(struct nd_neighbor_solicit)) > goto badlen; > if ((n = m_copym(m, 0, M_COPYALL, M_DONTWAIT)) == NULL) { > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - /* Send incoming SeND/ND packet to user space. */ > - printf("%s: send_sendso_input_hook m=%p\n", __func__, m); > - send_sendso_input_hook(V_send_so, m, SND_IN, ip6len); > - } else { > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(m, > + SND_IN, ip6len); > + } > + if ((send_sendso_input_hook != NULL > + && error == -1) || > + send_sendso_input_hook == NULL) { > /* give up local */ > nd6_ns_input(m, off, icmp6len); > } > m = NULL; > goto freeit; > } > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - /* Send incoming SeND/ND packet to user space. */ > - printf("%s: send_sendso_input_hook n=%p\n", __func__, n); > - send_sendso_input_hook(V_send_so, n, SND_IN, ip6len); > - } else > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(n, > + SND_IN, ip6len); > + } > + if ((send_sendso_input_hook != NULL && error == -1) > + || (send_sendso_input_hook == NULL)) { > nd6_ns_input(n, off, icmp6len); > + } > /* m stays. */ > break; > > @@ -863,20 +890,29 @@ > if ((n = m_copym(m, 0, M_COPYALL, M_DONTWAIT)) == NULL) { > > /* Send incoming SeND-protected/ND packet to user space. */ > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - send_sendso_input_hook(V_send_so, m, SND_IN, ip6len); > - return (IPPROTO_DONE); > - } else { > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(m, > + SND_IN, ip6len); > + if (!error) > + return (IPPROTO_DONE); > + } > + if ((send_sendso_input_hook != NULL > + && error == -1) || > + send_sendso_input_hook == NULL) { > /* give up local */ > nd6_na_input(m, off, icmp6len); > } > m = NULL; > goto freeit; > } > - if (send_sendso_input_hook != NULL) > - send_sendso_input_hook(V_send_so, n, SND_IN, ip6len); > - else > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(n, > + SND_IN, ip6len); > + } > + if ((send_sendso_input_hook != NULL && error == -1) > + || (send_sendso_input_hook == NULL)) { > nd6_na_input(n, off, icmp6len); > + } > /* m stays. */ > break; > > @@ -887,23 +923,35 @@ > if (icmp6len < sizeof(struct nd_redirect)) > goto badlen; > if ((n = m_copym(m, 0, M_COPYALL, M_DONTWAIT)) == NULL) { > - > - /* Send incoming SeND-protected/ND packet to user space. */ > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - send_sendso_input_hook(V_send_so, m, SND_IN, ip6len); > - return (IPPROTO_DONE); > - } else { > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(m, > + SND_IN, ip6len); > + if (!error) > + return (IPPROTO_DONE); > + else > + goto freeit; > + } > + if ((send_sendso_input_hook != NULL > + && error == -1) || > + send_sendso_input_hook == NULL) { > /* give up local */ > icmp6_redirect_input(m, off); > } > m = NULL; > goto freeit; > } > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - send_sendso_input_hook(V_send_so, n, SND_IN, ip6len); > - return (IPPROTO_DONE); > - } else > + if (send_sendso_input_hook != NULL) { > + error = send_sendso_input_hook(n, > + SND_IN, ip6len); > + if (!error) > + return (IPPROTO_DONE); > + else > + goto freeit; > + } > + if ((send_sendso_input_hook != NULL && error == -1) > + || (send_sendso_input_hook == NULL)) { > icmp6_redirect_input(n, off); > + } > /* m stays. */ > break; > > @@ -2805,7 +2853,7 @@ > nd_rd->nd_rd_cksum = in6_cksum(m, IPPROTO_ICMPV6, > sizeof(*ip6), ntohs(ip6->ip6_plen)); > > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > + if (send_sendso_input_hook != NULL) { > mtag = m_tag_get(PACKET_TAG_ND_OUTGOING, sizeof(unsigned short), > M_NOWAIT); > if (mtag == NULL) > > ==== //depot/projects/soc2009/anchie_send/src/sys/netinet6/nd6.c#29 > (text+ko) ==== > > @@ -113,7 +113,7 @@ > > static struct sockaddr_in6 all1_sa; > > -int (*send_sendso_input_hook)(struct socket *, struct mbuf *, int, int); > +int (*send_sendso_input_hook)(struct mbuf *, int, int); > > static int nd6_is_new_addr_neighbor __P((struct sockaddr_in6 *, > struct ifnet *)); > @@ -1803,9 +1803,10 @@ > struct m_tag *mtag; > struct llentry *ln = lle; > struct ip6_hdr *ip6; > - int error = 0; > + int error = -1; > int flags = 0; > - int ip6len, skip = 0; > + int ip6len; > + int skip; > unsigned short *nd_type; > > ip6 = mtod(m, struct ip6_hdr *); > @@ -1985,15 +1986,19 @@ > mac_netinet6_nd6_send(ifp, m); > #endif > > + skip = 0; > /* send outgoing NS/NA/REDIRECT packet to sendd. */ > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > + if (send_sendso_input_hook != NULL) { > mtag = m_tag_find(m, PACKET_TAG_ND_OUTGOING, NULL); > if (mtag != NULL) { > skip = 1; > nd_type = (unsigned short *)(mtag + 1); > /* Use the SEND socket */ > - return (send_sendso_input_hook(V_send_so, > - m, SND_OUT, ip6len); > + error = send_sendso_input_hook(m, SND_OUT, > + ip6len); > + /* -1 == no app on SEND socket */ > + if (error == 0 && error != -1) > + return (error); > } > } > > > ==== //depot/projects/soc2009/anchie_send/src/sys/netinet6/nd6_nbr.c#16 > (text+ko) ==== > > @@ -569,9 +569,9 @@ > nd_ns->nd_ns_cksum = > in6_cksum(m, IPPROTO_ICMPV6, sizeof(*ip6), icmp6len); > > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - mtag = m_tag_get(PACKET_TAG_ND_OUTGOING, sizeof(unsigned short), > - M_NOWAIT); > + if (send_sendso_input_hook != NULL) { > + mtag = m_tag_get(PACKET_TAG_ND_OUTGOING, > + sizeof(unsigned short), M_NOWAIT); > if (mtag == NULL) > goto bad; > *(unsigned short *)(mtag + 1) = nd_ns->nd_ns_type; > @@ -896,7 +896,7 @@ > * the 2nd argument as the 1st one. > */ > > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > + if (send_sendso_input_hook != NULL) { > mtag = m_tag_get(PACKET_TAG_ND_OUTGOING, > sizeof(unsigned short), M_NOWAIT); > if (mtag == NULL) > @@ -1091,8 +1091,9 @@ > nd_na->nd_na_cksum = > in6_cksum(m, IPPROTO_ICMPV6, sizeof(struct ip6_hdr), icmp6len); > > - if (send_sendso_input_hook != NULL && V_send_so != NULL) { > - mtag = m_tag_get(PACKET_TAG_ND_OUTGOING, sizeof(unsigned short), > + if (send_sendso_input_hook != NULL) { > + mtag = m_tag_get(PACKET_TAG_ND_OUTGOING, > + sizeof(unsigned short), > M_NOWAIT); > if (mtag == NULL) > goto bad; > > ==== //depot/projects/soc2009/anchie_send/src/sys/netinet6/raw_ip6.c#10 > (text+ko) ==== > > @@ -535,8 +535,7 @@ > * Send RA/RS messages to user land for protection, before sending > * them to rtadvd/rtsol. > */ > - if (send_sendso_input_hook != NULL && > - V_send_so != NULL && > + if ((send_sendso_input_hook != NULL) && > so->so_proto->pr_protocol == IPPROTO_ICMPV6) { > switch (type) { > case ND_ROUTER_ADVERT: > > ==== //depot/projects/soc2009/anchie_send/src/sys/netinet6/send.c#43 > (text+ko) ==== > > @@ -195,12 +195,15 @@ > * Send a message to the SEND daemon on the SEND socket. > */ > static int > -send_sendso_input(struct socket *s, struct mbuf *m, int direction, int > msglen) +send_sendso_input(struct mbuf *m, int direction, int msglen) > { > u_int len; > struct ip6_hdr *ip6; > struct snd_hdr *snd_hdr = NULL; > > + if (V_send_so == NULL) > + goto freeit; > + > /* > * Make sure to clear any possible internally embedded scope before > * passing the packet to userspace for SeND cryptographic signature > @@ -225,15 +228,14 @@ > * protected (outgoing) or validated (incoming) according to rfc3971. > */ > > - if (s) { > - SOCKBUF_LOCK(&s->so_rcv); > - sbappendrecord_locked(&s->so_rcv, m); > - sorwakeup_locked(s); > - return (0); > - } > + SOCKBUF_LOCK(&s->so_rcv); > + sbappendrecord_locked(&s->so_rcv, m); > + sorwakeup_locked(s); > + return (0); > > +freeit: > m_freem(m); > - return -1; > + return (-1); > } > > static void > > ==== //depot/projects/soc2009/anchie_send/src/sys/netinet6/send.h#21 > (text+ko) ==== > > @@ -34,4 +34,4 @@ > int ifidx; > }; > > -extern int (*send_sendso_input_hook)(struct socket *, struct mbuf *, > int, int); +extern int (*send_sendso_input_hook)(struct mbuf *, int, > int);