Skip site navigation (1)Skip section navigation (2) 
Date:      Sat, 23 Jan 2016 11:05:13 +0000 (UTC)
From:      "Bjoern A. Zeeb" <bz@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-projects@freebsd.org
Subject:   svn commit: r294622 - projects/vnet/sys/netinet
Message-ID:  <201601231105.u0NB5DAW072559@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: bz
Date: Sat Jan 23 11:05:13 2016
New Revision: 294622
URL: https://svnweb.freebsd.org/changeset/base/294622

Log:
  Try to catch a couple of SCTP teardown race conditions.
  Saw all the printfs already.
  
  Note: not sure the atomics are needed but without them, the condition
  would never trigger, and we'd still see panics (which could have been
  due to the insert race).  Will work my way backwards in case this stays
  stable.
  
  Sponsored by:	The FreeBSD Foundation

Modified:
  projects/vnet/sys/netinet/sctp_asconf.c
  projects/vnet/sys/netinet/sctp_bsd_addr.c
  projects/vnet/sys/netinet/sctp_pcb.c
  projects/vnet/sys/netinet/sctp_structs.h
  projects/vnet/sys/netinet/sctputil.c

Modified: projects/vnet/sys/netinet/sctp_asconf.c
==============================================================================
--- projects/vnet/sys/netinet/sctp_asconf.c	Sat Jan 23 08:08:06 2016	(r294621)
+++ projects/vnet/sys/netinet/sctp_asconf.c	Sat Jan 23 11:05:13 2016	(r294622)
@@ -3248,6 +3248,7 @@ sctp_addr_mgmt_ep_sa(struct sctp_inpcb *
 		} else {
 			struct sctp_asconf_iterator *asc;
 			struct sctp_laddr *wi;
+			int ret;
 
 			SCTP_MALLOC(asc, struct sctp_asconf_iterator *,
 			    sizeof(struct sctp_asconf_iterator),
@@ -3269,7 +3270,7 @@ sctp_addr_mgmt_ep_sa(struct sctp_inpcb *
 			wi->action = type;
 			atomic_add_int(&ifa->refcount, 1);
 			LIST_INSERT_HEAD(&asc->list_of_work, wi, sctp_nxt_addr);
-			(void)sctp_initiate_iterator(sctp_asconf_iterator_ep,
+			ret = sctp_initiate_iterator(sctp_asconf_iterator_ep,
 			    sctp_asconf_iterator_stcb,
 			    sctp_asconf_iterator_ep_end,
 			    SCTP_PCB_ANY_FLAGS,
@@ -3277,6 +3278,15 @@ sctp_addr_mgmt_ep_sa(struct sctp_inpcb *
 			    SCTP_ASOC_ANY_STATE,
 			    (void *)asc, 0,
 			    sctp_asconf_iterator_end, inp, 0);
+			if (ret)  {
+				SCTP_PRINTF("Failed to initiate iterator for addr_mgmt_ep_sa\n");
+				SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_ASCONF, EFAULT);
+				atomic_add_int(&ifa->refcount, -1);
+				SCTP_DECR_LADDR_COUNT();
+				SCTP_ZONE_FREE(SCTP_BASE_INFO(ipi_zone_laddr), wi);	
+				SCTP_FREE(asc, SCTP_M_ASC_IT);
+				return (EFAULT);
+			}
 		}
 		return (0);
 	} else {

Modified: projects/vnet/sys/netinet/sctp_bsd_addr.c
==============================================================================
--- projects/vnet/sys/netinet/sctp_bsd_addr.c	Sat Jan 23 08:08:06 2016	(r294621)
+++ projects/vnet/sys/netinet/sctp_bsd_addr.c	Sat Jan 23 11:05:13 2016	(r294622)
@@ -77,7 +77,7 @@ struct iterator_control sctp_it_ctl;
 void
 sctp_wakeup_iterator(void)
 {
-	wakeup(&sctp_it_ctl.iterator_running);
+	wakeup(&sctp_it_ctl.iterator_flags);
 }
 
 static void
@@ -86,7 +86,7 @@ sctp_iterator_thread(void *v SCTP_UNUSED
 	SCTP_IPI_ITERATOR_WQ_LOCK();
 	/* In FreeBSD this thread never terminates. */
 	for (;;) {
-		msleep(&sctp_it_ctl.iterator_running,
+		msleep(&sctp_it_ctl.iterator_flags,
 		    &sctp_it_ctl.ipi_iterator_wq_mtx,
 		    0, "waiting_for_work", 0);
 		sctp_iterator_worker();

Modified: projects/vnet/sys/netinet/sctp_pcb.c
==============================================================================
--- projects/vnet/sys/netinet/sctp_pcb.c	Sat Jan 23 08:08:06 2016	(r294621)
+++ projects/vnet/sys/netinet/sctp_pcb.c	Sat Jan 23 11:05:13 2016	(r294622)
@@ -5929,6 +5929,7 @@ sctp_pcb_finish(void)
 	struct sctp_tagblock *twait_block, *prev_twait_block;
 	struct sctp_laddr *wi, *nwi;
 	int i;
+	unsigned int r;
 	struct sctp_iterator *it, *nit;
 
 	if (SCTP_BASE_VAR(sctp_pcb_initialized) == 0) {
@@ -5943,8 +5944,6 @@ sctp_pcb_finish(void)
 	 * still add the ifdef to make it compile on old versions.
 	 */
 retry:
-	while (sctp_it_ctl.iterator_running != 0)
-		DELAY(1);
 	SCTP_IPI_ITERATOR_WQ_LOCK();
 	/*
 	 * sctp_iterator_worker() might be working on an it entry without
@@ -5953,10 +5952,13 @@ retry:
 	 * avoid the race condition as sctp_iterator_worker() will have to
 	 * wait to re-aquire the lock.
 	 */
-	if (sctp_it_ctl.cur_it != NULL || sctp_it_ctl.iterator_running != 0) {
+	r = atomic_fetchadd_int(&sctp_it_ctl.iterator_running, 0);
+	if (r != 0 || sctp_it_ctl.cur_it != NULL) {
 		SCTP_IPI_ITERATOR_WQ_UNLOCK();
-		printf("%s: Iterator running while we held the lock. Retry.\n",
-		    __func__);
+		/* XXX-BZ make this a statistics variable. */
+		printf("%s: Iterator running while we held the lock. Retry. "
+		    "r=%u cur_it=%p\n", __func__, r, sctp_it_ctl.cur_it);
+		DELAY(10);
 		goto retry;
 	}
 	TAILQ_FOREACH_SAFE(it, &sctp_it_ctl.iteratorhead, sctp_nxt_itr, nit) {
@@ -7022,6 +7024,11 @@ sctp_initiate_iterator(inp_func inpf,
 	if (af == NULL) {
 		return (-1);
 	}
+	if (SCTP_BASE_VAR(sctp_pcb_initialized) == 0) {
+		printf("%s: abort on initialize being %d\n", __func__,
+		    SCTP_BASE_VAR(sctp_pcb_initialized));
+		return (-1);
+	}
 	SCTP_MALLOC(it, struct sctp_iterator *, sizeof(struct sctp_iterator),
 	    SCTP_M_ITER);
 	if (it == NULL) {
@@ -7060,9 +7067,16 @@ sctp_initiate_iterator(inp_func inpf,
 
 	}
 	SCTP_IPI_ITERATOR_WQ_LOCK();
+	if (SCTP_BASE_VAR(sctp_pcb_initialized) == 0) {
+		SCTP_IPI_ITERATOR_WQ_UNLOCK();
+		printf("%s: rollback on initialize being %d it=%p\n", __func__,
+		    SCTP_BASE_VAR(sctp_pcb_initialized), it);
+		SCTP_FREE(it, SCTP_M_ITER);
+		return (-1);
+	}
 
 	TAILQ_INSERT_TAIL(&sctp_it_ctl.iteratorhead, it, sctp_nxt_itr);
-	if (sctp_it_ctl.iterator_running == 0) {
+	if (atomic_fetchadd_int(&sctp_it_ctl.iterator_running, 0) == 0) {
 		sctp_wakeup_iterator();
 	}
 	SCTP_IPI_ITERATOR_WQ_UNLOCK();

Modified: projects/vnet/sys/netinet/sctp_structs.h
==============================================================================
--- projects/vnet/sys/netinet/sctp_structs.h	Sat Jan 23 08:08:06 2016	(r294621)
+++ projects/vnet/sys/netinet/sctp_structs.h	Sat Jan 23 11:05:13 2016	(r294622)
@@ -180,7 +180,7 @@ struct iterator_control {
 	SCTP_PROCESS_STRUCT thread_proc;
 	struct sctpiterators iteratorhead;
 	struct sctp_iterator *cur_it;
-	uint32_t iterator_running;
+	volatile uint32_t iterator_running;
 	uint32_t iterator_flags;
 };
 

Modified: projects/vnet/sys/netinet/sctputil.c
==============================================================================
--- projects/vnet/sys/netinet/sctputil.c	Sat Jan 23 08:08:06 2016	(r294621)
+++ projects/vnet/sys/netinet/sctputil.c	Sat Jan 23 11:05:13 2016	(r294622)
@@ -1421,7 +1421,7 @@ sctp_iterator_worker(void)
 
 	/* This function is called with the WQ lock in place */
 
-	sctp_it_ctl.iterator_running = 1;
+	atomic_store_rel_int(&sctp_it_ctl.iterator_running, 1);
 	TAILQ_FOREACH_SAFE(it, &sctp_it_ctl.iteratorhead, sctp_nxt_itr, nit) {
 		sctp_it_ctl.cur_it = it;
 		/* now lets work on this one */
@@ -1434,7 +1434,7 @@ sctp_iterator_worker(void)
 		SCTP_IPI_ITERATOR_WQ_LOCK();
 		/* sa_ignore FREED_MEMORY */
 	}
-	sctp_it_ctl.iterator_running = 0;
+	atomic_store_rel_int(&sctp_it_ctl.iterator_running, 0);
 	return;
 }

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