Subject: [review please] tcp syn cache cleanup code for sc->sc_so
To: None <tech-net@netbsd.org>
From: None <itojun@iijlab.net>
List: tech-net
Date: 08/20/1999 14:03:59
	Hello, could someone review the following patch.

	By addition of IPsec code, tcp syn cache has additional member,
	sc_so, which points back to socket structure of listening socket.
	This is required because, when responding to the connection attempt
	at syn_cache_respond(), we need to check the security policy
	of relevant listening socket ("need encryption on reply" for example).

	There can be little chance of race condition, when:
	- you run web server (listening socket on port 80)
	- you received SYN on port 80, created syn cache
	- web server is killed (by signal, for example) and listening socket
	  goes away
	- sc_so on syn cache still has dangling reference at sc_so, and
	  may cause some trouble
	First of all I'm guilty for the bug, I'm sorry about this.

	One obvious fix to this is to totally forget about syn cache, which is
	not acceptable.
	Another fix would be to forget about syn cache (make copy of
	socket structure right after SYN) when ipsec policy is associated
	to the socket - this may not be desirable and addes too many confusion
	in the code.

	The following patch does third option - tries to cleanup reference
	to listening socket from syn cache entries when listening socket
	goes away.  This may not show the best behavior (if listening socket
	goes away, ipsec policy will become inaccessible), but this should
	protect kenrel from panic'ing.
	I would like to know if this fix looks feasible enough.

	The diff is based on KAME repository so there may be some difference
	in line numbers.  Basically tcp_close() calls syn_cache_cleanup()
	which checks every entries in syn cache for to-be-dangling pointer.

	Relationship betwen syn cache and per-socket ipsec policy is,
	I would say, very tricky...

itojun



Index: tcp_input.c
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/sys/netinet/tcp_input.c,v
retrieving revision 1.5
diff -c -r1.5 tcp_input.c
*** tcp_input.c	1999/08/19 12:11:59	1.5
--- tcp_input.c	1999/08/20 04:53:01
***************
*** 2545,2550 ****
--- 2545,2574 ----
  }
  
  /*
+  * Remove reference from syn cache to socket structure,
+  * before socket strucuture goes away.
+  */
+ void
+ syn_cache_cleanup(so)
+ 	struct socket *so;
+ {
+ 	struct syn_cache *sc;
+ 	int i, s;
+ 
+ 	s = splsoftnet();
+ 
+ 	for (i = 0; i < TCP_MAXRXTSHIFT; i++) {
+ 		for (sc = TAILQ_FIRST(&tcp_syn_cache_timeq[i]);
+ 		     sc != NULL;
+ 		     sc = TAILQ_NEXT(sc, sc_timeq)) {
+ 			if (sc->sc_so == so)
+ 				sc->sc_so = NULL;
+ 		}
+ 	}
+ 	splx(s);
+ }
+ 
+ /*
   * Find an entry in the syn cache.
   */
  struct syn_cache *
Index: tcp_subr.c
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/sys/netinet/tcp_subr.c,v
retrieving revision 1.5
diff -c -r1.5 tcp_subr.c
*** tcp_subr.c	1999/08/11 06:04:32	1.5
--- tcp_subr.c	1999/08/20 04:53:01
***************
*** 905,916 ****
--- 905,918 ----
  	pool_put(&tcpcb_pool, tp);
  	if (inp) {
  		inp->inp_ppcb = 0;
+ 		syn_cache_cleanup(so);
  		soisdisconnected(so);
  		in_pcbdetach(inp);
  	}
  #ifdef INET6
  	else if (in6p) {
  		in6p->in6p_ppcb = 0;
+ 		syn_cache_cleanup(so);
  		soisdisconnected(so);
  		in6_pcbdetach(in6p);
  	}
Index: tcp_var.h
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/sys/netinet/tcp_var.h,v
retrieving revision 1.4
diff -c -r1.4 tcp_var.h
*** tcp_var.h	1999/08/12 13:58:16	1.4
--- tcp_var.h	1999/08/20 04:53:01
***************
*** 686,691 ****
--- 686,692 ----
  		struct tcphdr *));
  int	 syn_cache_respond __P((struct syn_cache *, struct mbuf *));
  void	 syn_cache_timer __P((void));
+ void	 syn_cache_cleanup __P((struct socket *));
  
  int	tcp_newreno __P((struct tcpcb *, struct tcphdr *));
  #endif