From owner-svn-src-head@FreeBSD.ORG Tue Jan 6 12:43:42 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 7C507106566B; Tue, 6 Jan 2009 12:43:42 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 3B1638FC14; Tue, 6 Jan 2009 12:43:42 +0000 (UTC) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [65.122.17.41]) by cyrus.watson.org (Postfix) with ESMTPS id D508A46B3C; Tue, 6 Jan 2009 07:43:41 -0500 (EST) Date: Tue, 6 Jan 2009 12:43:41 +0000 (GMT) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: Randall Stewart In-Reply-To: <200901061213.n06CDfuv030333@svn.freebsd.org> Message-ID: References: <200901061213.n06CDfuv030333@svn.freebsd.org> User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, 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 12:43:42 -0000 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. > - 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); > - 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? 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); >