Date: Tue, 6 Jan 2009 08:19:24 -0500 From: Randall Stewart <rrs@lakerest.net> To: Robert Watson <rwatson@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, Randall Stewart <rrs@FreeBSD.org>, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r186813 - in head/sys: netinet netinet6 Message-ID: <13F887DB-F42B-4283-B19A-02CCCB8CDB27@lakerest.net> In-Reply-To: <alpine.BSF.2.00.0901061238290.8129@fledge.watson.org> References: <200901061213.n06CDfuv030333@svn.freebsd.org> <alpine.BSF.2.00.0901061238290.8129@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?13F887DB-F42B-4283-B19A-02CCCB8CDB27>