svn commit: r186813 - in head/sys: netinet netinet6

[Robert Watson]

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);
>