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>