From owner-svn-src-head@FreeBSD.ORG Tue Jan 6 13:19:27 2009 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0BCEF106564A; Tue, 6 Jan 2009 13:19:27 +0000 (UTC) (envelope-from rrs@lakerest.net) Received: from lakerest.net (unknown [IPv6:2001:240:585:2:203:6dff:fe1a:4ddc]) by mx1.freebsd.org (Postfix) with ESMTP id AAC678FC0C; Tue, 6 Jan 2009 13:19:26 +0000 (UTC) (envelope-from rrs@lakerest.net) Received: from [10.1.1.54] ([10.1.1.54]) (authenticated bits=0) by lakerest.net (8.14.1/8.14.1) with ESMTP id n06DJOI4059166 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Tue, 6 Jan 2009 08:19:25 -0500 (EST) (envelope-from rrs@lakerest.net) Message-Id: <13F887DB-F42B-4283-B19A-02CCCB8CDB27@lakerest.net> From: Randall Stewart To: Robert Watson In-Reply-To: Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit Mime-Version: 1.0 (Apple Message framework v930.3) Date: Tue, 6 Jan 2009 08:19:24 -0500 References: <200901061213.n06CDfuv030333@svn.freebsd.org> X-Mailer: Apple Mail (2.930.3) Cc: svn-src-head@FreeBSD.org, Randall Stewart , svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r186813 - in head/sys: netinet netinet6 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Jan 2009 13:19:27 -0000 On Jan 6, 2009, at 7:43 AM, Robert Watson wrote: > On Tue, 6 Jan 2009, Randall Stewart wrote: > >> + /* >> + * Engage the tunneling protocol we >> + * will have to leave the info_lock >> + * up, since we are hunting through >> + * multiple UDP inp's hope we don't >> + * break. >> + * >> + * XXXML: Maybe add a flag to the >> + * prototype so that the tunneling >> + * can defer work that can't be done >> + * under the info lock? >> + */ >> + udp_tun_func_t tunnel_func; >> + >> + tunnel_func = (udp_tun_func_t)last->inp_ppcb; >> + tunnel_func(n, iphlen, last); >> + INP_RUNLOCK(last); > > There's a basic working assumption throughout most protocol input > paths that the inpcb (and hence things it points to) will remain > valid only if appropriate locks are held, so I think we won't ever > be in a position where we can safely call tunnel_func() without at > least the inpcb, and possibly the pcbinfo, lock. As such, I think > we can lose the comments about locking here and elsewhere. Right.. Ok I will scrub out all the stale comments.. > > >> - udp_append(last, ip, m, iphlen + sizeof(struct udphdr), >> - &udp_in); >> - INP_RUNLOCK(last); >> - INP_INFO_RUNLOCK(&V_udbinfo); >> + if (last->inp_ppcb == NULL) { >> + udp_append(last, ip, m, iphlen + sizeof(struct udphdr), >> + &udp_in); >> + INP_RUNLOCK(last); >> + INP_INFO_RUNLOCK(&V_udbinfo); >> + } else { >> + /* >> + * Engage the tunneling protocol we must make sure >> + * all locks are released when we call the tunneling >> + * protocol. > > This comment appears to be stale -- the locks are held, and must be. > >> + /* >> + * Engage the tunneling protocol we must make sure all locks >> + * are released when we call the tunneling protocol. >> + */ >> + udp_tun_func_t tunnel_func; > > Ditto here -- locks are held, and must be. > >> +int >> +udp_set_kernel_tunneling(struct socket *so, udp_tun_func_t f) >> +{ >> + struct inpcb *inp; >> + > > Hmm. Perhaps the sanity tests here should be KASSERT's: > > KASSERT(so->so_type == SOCK_DGRAM, ("udp_set_kernel_tunneling: ! > dgram")); > KASSERT(so->so_ppcb != NULL, ("udp_set_kernel_tunneling: NULL inp")); > > Otherwise these if's are really just error handling for violated > invariants. > >> + INP_WLOCK(inp); >> + inp->inp_ppcb = f; >> INP_WUNLOCK(inp); > > However, I could see having some error-handling here, perhaps > something like: > > INP_WLOCK(inp); > if (inp->inp_ppcb != NULL) { > INP_WUNLOCK(inp); > return (EBUSY); > } > inp->inp_ppcb = f; > INP_WUNLOCK(inp); > Hmm yeah a KASSERT would be better. And I like your error check.. I will add these with the commit. >> - udp6_append(last, n, off, &fromsa); >> - INP_RUNLOCK(last); >> + if (last->inp_ppcb != NULL) { >> + /* >> + * Engage the tunneling >> + * protocol we will have to >> + * leave the info_lock up, >> + * since we are hunting >> + * through multiple UDP >> + * inp's hope we don't break. > > I think we can omit the comment here -- the locks are necessary and > unavoidable, so any functions called here need to deal with it by > definition. > >> + if (last->inp_ppcb != NULL) { >> + /* >> + * Engage the tunneling protocol we must make sure >> + * all locks are released when we call the tunneling >> + * protocol. > > Ditto as above -- stale, and probably unnecessary. > >> @@ -354,6 +384,18 @@ udp6_input(struct mbuf **mp, int *offp, >> } >> INP_RLOCK(inp); >> INP_INFO_RUNLOCK(&V_udbinfo); >> + if (inp->inp_ppcb != NULL) { >> + /* >> + * Engage the tunneling protocol we must make sure all locks >> + * are released when we call the tunneling protocol. >> + */ > > Ditto. BTW, have you thought about trying to use so_upcall for > these things rather than something specific to the UDP layer, or do > you really need the full IP/UDP header? > I think the answer is yes, we definitely need the full IP header and UDP header. Consider that the transport protocol you are tunneling to will do (in general) the following: 1) Pull out the UDP port number of the sender and stash this (or make sure it is stashed) in its internal PCB. 2) Strip out the udp header. 3) Present the IP+Real Transport header AND the udp Port used (to act as a flag that it came via udp) to the real transports NNN_input() function. Without the full IP/UDP headers you would not be able to do this. R > Robert N M Watson > Computer Laboratory > University of Cambridge > >> + udp_tun_func_t tunnel_func; >> + >> + tunnel_func = (udp_tun_func_t)inp->inp_ppcb; >> + tunnel_func(m, off, inp); >> + INP_RUNLOCK(inp); >> + return (IPPROTO_DONE); >> + } >> udp6_append(inp, m, off, &fromsa); >> INP_RUNLOCK(inp); >> return (IPPROTO_DONE); >> > ------------------------------ Randall Stewart 803-317-4952 (cell) 803-345-0391(direct)