svn commit: r186813 - in head/sys: netinet netinet6
Robert Watson
rwatson at FreeBSD.org
Tue Jan 6 04:43:42 PST 2009
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);
>
More information about the svn-src-all
mailing list