Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 11 Jun 2010 20:38:20 +0000 (UTC)
From:      Michael Tuexen <tuexen@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r209067 - stable/8/sys/netinet
Message-ID:  <201006112038.o5BKcK83083694@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: tuexen
Date: Fri Jun 11 20:38:20 2010
New Revision: 209067
URL: http://svn.freebsd.org/changeset/base/209067

Log:
  MFC 209029
  
   3 Fixes -
   a) There was a case where a ICMP message could cause
      us to return leaving a stuck lock on an stcb.
   b) The iterator needed some tweaks to fix its lock
      ordering.
   c) The ITERATOR_LOCK is no longer needed in the freeing
      of a stcb. Now that the timer based one is gone we don't
      have a multiple resume situation. Add to that that there
      was somewhere a path out of the freeing of an assoc that
      did NOT release the iterator_lock.. it was time to clean
      this old code up and in the process fix the lock bug.
  
  Approved by: re (bz)

Modified:
  stable/8/sys/netinet/sctp_pcb.c
  stable/8/sys/netinet/sctp_usrreq.c
  stable/8/sys/netinet/sctputil.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/xen/xenpci/   (props changed)
  stable/8/sys/geom/sched/   (props changed)

Modified: stable/8/sys/netinet/sctp_pcb.c
==============================================================================
--- stable/8/sys/netinet/sctp_pcb.c	Fri Jun 11 20:08:20 2010	(r209066)
+++ stable/8/sys/netinet/sctp_pcb.c	Fri Jun 11 20:38:20 2010	(r209067)
@@ -3075,6 +3075,11 @@ sctp_iterator_inp_being_freed(struct sct
 			} else {
 				it->inp = LIST_NEXT(it->inp, sctp_list);
 			}
+			/*
+			 * When its put in the refcnt is incremented so decr
+			 * it
+			 */
+			SCTP_INP_DECR_REF(inp);
 		}
 		it = nit;
 	}
@@ -4428,38 +4433,6 @@ sctp_add_vtag_to_timewait(uint32_t tag, 
 }
 
 
-static void
-sctp_iterator_asoc_being_freed(struct sctp_inpcb *inp, struct sctp_tcb *stcb)
-{
-	struct sctp_iterator *it;
-
-	/*
-	 * Unlock the tcb lock we do this so we avoid a dead lock scenario
-	 * where the iterator is waiting on the TCB lock and the TCB lock is
-	 * waiting on the iterator lock.
-	 */
-	it = stcb->asoc.stcb_starting_point_for_iterator;
-	if (it == NULL) {
-		return;
-	}
-	if (it->inp != stcb->sctp_ep) {
-		/* hmm, focused on the wrong one? */
-		return;
-	}
-	if (it->stcb != stcb) {
-		return;
-	}
-	it->stcb = LIST_NEXT(stcb, sctp_tcblist);
-	if (it->stcb == NULL) {
-		/* done with all asoc's in this assoc */
-		if (it->iterator_flags & SCTP_ITERATOR_DO_SINGLE_INP) {
-			it->inp = NULL;
-		} else {
-			it->inp = LIST_NEXT(inp, sctp_list);
-		}
-	}
-}
-
 
 /*-
  * Free the association after un-hashing the remote port. This
@@ -4665,7 +4638,6 @@ sctp_free_assoc(struct sctp_inpcb *inp, 
 
 		SCTP_TCB_UNLOCK(stcb);
 
-		SCTP_ITERATOR_LOCK();
 		SCTP_INP_INFO_WLOCK();
 		SCTP_INP_WLOCK(inp);
 		SCTP_TCB_LOCK(stcb);
@@ -4704,7 +4676,6 @@ sctp_free_assoc(struct sctp_inpcb *inp, 
 	 * Make it invalid too, that way if its about to run it will abort
 	 * and return.
 	 */
-	sctp_iterator_asoc_being_freed(inp, stcb);
 	/* re-increment the lock */
 	if (from_inpcbfree == SCTP_NORMAL_PROC) {
 		atomic_add_int(&stcb->asoc.refcnt, -1);
@@ -4721,7 +4692,6 @@ sctp_free_assoc(struct sctp_inpcb *inp, 
 	if (from_inpcbfree == SCTP_NORMAL_PROC) {
 		SCTP_INP_INCR_REF(inp);
 		SCTP_INP_WUNLOCK(inp);
-		SCTP_ITERATOR_UNLOCK();
 	}
 	/* pull from vtag hash */
 	LIST_REMOVE(stcb, sctp_asocs);
@@ -6694,20 +6664,22 @@ sctp_initiate_iterator(inp_func inpf,
 	it->no_chunk_output = chunk_output_off;
 	it->vn = curvnet;
 	if (s_inp) {
+		/* Assume lock is held here */
 		it->inp = s_inp;
+		SCTP_INP_INCR_REF(it->inp);
 		it->iterator_flags = SCTP_ITERATOR_DO_SINGLE_INP;
 	} else {
 		SCTP_INP_INFO_RLOCK();
 		it->inp = LIST_FIRST(&SCTP_BASE_INFO(listhead));
-
+		if (it->inp) {
+			SCTP_INP_INCR_REF(it->inp);
+		}
 		SCTP_INP_INFO_RUNLOCK();
 		it->iterator_flags = SCTP_ITERATOR_DO_ALL_INP;
 
 	}
 	SCTP_IPI_ITERATOR_WQ_LOCK();
-	if (it->inp) {
-		SCTP_INP_INCR_REF(it->inp);
-	}
+
 	TAILQ_INSERT_TAIL(&sctp_it_ctl.iteratorhead, it, sctp_nxt_itr);
 	if (sctp_it_ctl.iterator_running == 0) {
 		sctp_wakeup_iterator();

Modified: stable/8/sys/netinet/sctp_usrreq.c
==============================================================================
--- stable/8/sys/netinet/sctp_usrreq.c	Fri Jun 11 20:08:20 2010	(r209066)
+++ stable/8/sys/netinet/sctp_usrreq.c	Fri Jun 11 20:38:20 2010	(r209067)
@@ -402,6 +402,9 @@ sctp_ctlinput(cmd, sa, vip)
 				SCTP_INP_DECR_REF(inp);
 				SCTP_INP_WUNLOCK(inp);
 			}
+			if (stcb) {
+				SCTP_TCB_UNLOCK(stcb);
+			}
 		}
 	}
 	return;

Modified: stable/8/sys/netinet/sctputil.c
==============================================================================
--- stable/8/sys/netinet/sctputil.c	Fri Jun 11 20:08:20 2010	(r209066)
+++ stable/8/sys/netinet/sctputil.c	Fri Jun 11 20:38:20 2010	(r209067)
@@ -1255,15 +1255,20 @@ sctp_iterator_work(struct sctp_iterator 
 {
 	int iteration_count = 0;
 	int inp_skip = 0;
+	int first_in = 1;
+	struct sctp_inpcb *tinp;
 
+	SCTP_INP_INFO_RLOCK();
 	SCTP_ITERATOR_LOCK();
 	if (it->inp) {
+		SCTP_INP_RLOCK(it->inp);
 		SCTP_INP_DECR_REF(it->inp);
 	}
 	if (it->inp == NULL) {
 		/* iterator is complete */
 done_with_iterator:
 		SCTP_ITERATOR_UNLOCK();
+		SCTP_INP_INFO_RUNLOCK();
 		if (it->function_atend != NULL) {
 			(*it->function_atend) (it->pointer, it->val);
 		}
@@ -1271,7 +1276,11 @@ done_with_iterator:
 		return;
 	}
 select_a_new_ep:
-	SCTP_INP_RLOCK(it->inp);
+	if (first_in) {
+		first_in = 0;
+	} else {
+		SCTP_INP_RLOCK(it->inp);
+	}
 	while (((it->pcb_flags) &&
 	    ((it->inp->sctp_flags & it->pcb_flags) != it->pcb_flags)) ||
 	    ((it->pcb_features) &&
@@ -1281,8 +1290,9 @@ select_a_new_ep:
 			SCTP_INP_RUNLOCK(it->inp);
 			goto done_with_iterator;
 		}
-		SCTP_INP_RUNLOCK(it->inp);
+		tinp = it->inp;
 		it->inp = LIST_NEXT(it->inp, sctp_list);
+		SCTP_INP_RUNLOCK(tinp);
 		if (it->inp == NULL) {
 			goto done_with_iterator;
 		}
@@ -1323,6 +1333,8 @@ select_a_new_ep:
 			SCTP_INP_INCR_REF(it->inp);
 			SCTP_INP_RUNLOCK(it->inp);
 			SCTP_ITERATOR_UNLOCK();
+			SCTP_INP_INFO_RUNLOCK();
+			SCTP_INP_INFO_RLOCK();
 			SCTP_ITERATOR_LOCK();
 			if (sctp_it_ctl.iterator_flags) {
 				/* We won't be staying here */
@@ -1382,9 +1394,7 @@ no_stcb:
 	if (it->iterator_flags & SCTP_ITERATOR_DO_SINGLE_INP) {
 		it->inp = NULL;
 	} else {
-		SCTP_INP_INFO_RLOCK();
 		it->inp = LIST_NEXT(it->inp, sctp_list);
-		SCTP_INP_INFO_RUNLOCK();
 	}
 	if (it->inp == NULL) {
 		goto done_with_iterator;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201006112038.o5BKcK83083694>