Skip site navigation (1)Skip section navigation (2) 
Date:      Wed, 2 Dec 2020 00:48:15 +0000 (UTC)
From:      Mateusz Guzik <mjg@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r368271 - head/sys/kern
Message-ID:  <202012020048.0B20mFQo098578@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: mjg
Date: Wed Dec  2 00:48:15 2020
New Revision: 368271
URL: https://svnweb.freebsd.org/changeset/base/368271

Log:
  select: make sure there are no wakeup attempts after selfdfree returns
  
  Prior to the patch returning selfdfree could still be racing against doselwakeup
  which set sf_si = NULL and now locks stp to wake up the other thread.
  
  A sufficiently unlucky pair can end up going all the way down to freeing
  select-related structures before the lock/wakeup/unlock finishes.
  
  This started manifesting itself as crashes since select data started getting
  freed in r367714.

Modified:
  head/sys/kern/sys_generic.c

Modified: head/sys/kern/sys_generic.c
==============================================================================
--- head/sys/kern/sys_generic.c	Wed Dec  2 00:45:35 2020	(r368270)
+++ head/sys/kern/sys_generic.c	Wed Dec  2 00:48:15 2020	(r368271)
@@ -1820,14 +1820,17 @@ doselwakeup(struct selinfo *sip, int pri)
 		 */
 		TAILQ_REMOVE(&sip->si_tdlist, sfp, sf_threads);
 		stp = sfp->sf_td;
-		/*
-		 * Paired with selfdfree.
-		 */
-		atomic_store_rel_ptr((uintptr_t *)&sfp->sf_si, (uintptr_t)NULL);
 		mtx_lock(&stp->st_mtx);
 		stp->st_flags |= SELTD_PENDING;
 		cv_broadcastpri(&stp->st_wait, pri);
 		mtx_unlock(&stp->st_mtx);
+		/*
+		 * Paired with selfdfree.
+		 *
+		 * Storing this only after the wakeup provides an invariant that
+		 * stp is not used after selfdfree returns.
+		 */
+		atomic_store_rel_ptr((uintptr_t *)&sfp->sf_si, (uintptr_t)NULL);
 	}
 	mtx_unlock(sip->si_mtx);
 }
@@ -1837,14 +1840,18 @@ seltdinit(struct thread *td)
 {
 	struct seltd *stp;
 
-	if ((stp = td->td_sel) != NULL)
-		goto out;
-	td->td_sel = stp = malloc(sizeof(*stp), M_SELECT, M_WAITOK|M_ZERO);
+	stp = td->td_sel;
+	if (stp != NULL) {
+		MPASS(stp->st_flags == 0);
+		MPASS(STAILQ_EMPTY(&stp->st_selq));
+		return;
+	}
+	stp = malloc(sizeof(*stp), M_SELECT, M_WAITOK|M_ZERO);
 	mtx_init(&stp->st_mtx, "sellck", NULL, MTX_DEF);
 	cv_init(&stp->st_wait, "select");
-out:
 	stp->st_flags = 0;
 	STAILQ_INIT(&stp->st_selq);
+	td->td_sel = stp;
 }
 
 static int
@@ -1887,6 +1894,8 @@ seltdfini(struct thread *td)
 	stp = td->td_sel;
 	if (stp == NULL)
 		return;
+	MPASS(stp->st_flags == 0);
+	MPASS(STAILQ_EMPTY(&stp->st_selq));
 	if (stp->st_free1)
 		free(stp->st_free1, M_SELFD);
 	if (stp->st_free2)

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