From owner-svn-src-projects@freebsd.org Sat Jan 23 11:05:15 2016 Return-Path: Delivered-To: svn-src-projects@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 28112A8E635 for ; Sat, 23 Jan 2016 11:05:15 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 037EF19B5; Sat, 23 Jan 2016 11:05:14 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id u0NB5Ecb072564; Sat, 23 Jan 2016 11:05:14 GMT (envelope-from bz@FreeBSD.org) Received: (from bz@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id u0NB5DAW072559; Sat, 23 Jan 2016 11:05:13 GMT (envelope-from bz@FreeBSD.org) Message-Id: <201601231105.u0NB5DAW072559@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: bz set sender to bz@FreeBSD.org using -f From: "Bjoern A. Zeeb" Date: Sat, 23 Jan 2016 11:05:13 +0000 (UTC) To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r294622 - projects/vnet/sys/netinet X-SVN-Group: projects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Jan 2016 11:05:15 -0000 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; }