Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 24 Aug 2020 09:10:19 +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-12@freebsd.org
Subject:   svn commit: r364639 - stable/12/sys/netinet
Message-ID:  <202008240910.07O9AJ6n097082@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: tuexen
Date: Mon Aug 24 09:10:19 2020
New Revision: 364639
URL: https://svnweb.freebsd.org/changeset/base/364639

Log:
  MFC r363275:
  Improve the locking of address lists by adding some asserts and
  rearranging the addition of address such that the lock is not
  given up during checking and adding.

Modified:
  stable/12/sys/netinet/sctp_lock_bsd.h
  stable/12/sys/netinet/sctp_pcb.c
  stable/12/sys/netinet/sctp_usrreq.c
  stable/12/sys/netinet/sctputil.c
Directory Properties:
  stable/12/   (props changed)

Modified: stable/12/sys/netinet/sctp_lock_bsd.h
==============================================================================
--- stable/12/sys/netinet/sctp_lock_bsd.h	Mon Aug 24 09:06:46 2020	(r364638)
+++ stable/12/sys/netinet/sctp_lock_bsd.h	Mon Aug 24 09:10:19 2020	(r364639)
@@ -177,6 +177,13 @@ __FBSDID("$FreeBSD$");
 	rw_wunlock(&SCTP_BASE_INFO(ipi_addr_mtx));			\
 } while (0)
 
+#define SCTP_IPI_ADDR_LOCK_ASSERT() do {				\
+	rw_assert(&SCTP_BASE_INFO(ipi_addr_mtx), RA_LOCKED);		\
+} while (0)
+
+#define SCTP_IPI_ADDR_WLOCK_ASSERT() do {				\
+	rw_assert(&SCTP_BASE_INFO(ipi_addr_mtx), RA_WLOCKED);		\
+} while (0)
 
 #define SCTP_IPI_ITERATOR_WQ_INIT() do {				\
 	mtx_init(&sctp_it_ctl.ipi_iterator_wq_mtx, "sctp-it-wq",	\

Modified: stable/12/sys/netinet/sctp_pcb.c
==============================================================================
--- stable/12/sys/netinet/sctp_pcb.c	Mon Aug 24 09:06:46 2020	(r364638)
+++ stable/12/sys/netinet/sctp_pcb.c	Mon Aug 24 09:10:19 2020	(r364639)
@@ -200,6 +200,7 @@ sctp_find_ifn(void *ifn, uint32_t ifn_index)
 	 * We assume the lock is held for the addresses if that's wrong
 	 * problems could occur :-)
 	 */
+	SCTP_IPI_ADDR_LOCK_ASSERT();
 	hash_ifn_head = &SCTP_BASE_INFO(vrf_ifn_hash)[(ifn_index & SCTP_BASE_INFO(vrf_ifn_hashmark))];
 	LIST_FOREACH(sctp_ifnp, hash_ifn_head, next_bucket) {
 		if (sctp_ifnp->ifn_index == ifn_index) {
@@ -295,12 +296,16 @@ sctp_delete_ifn(struct sctp_ifn *sctp_ifnp, int hold_a
 		/* Not in the list.. sorry */
 		return;
 	}
-	if (hold_addr_lock == 0)
+	if (hold_addr_lock == 0) {
 		SCTP_IPI_ADDR_WLOCK();
+	} else {
+		SCTP_IPI_ADDR_WLOCK_ASSERT();
+	}
 	LIST_REMOVE(sctp_ifnp, next_bucket);
 	LIST_REMOVE(sctp_ifnp, next_ifn);
-	if (hold_addr_lock == 0)
+	if (hold_addr_lock == 0) {
 		SCTP_IPI_ADDR_WUNLOCK();
+	}
 	/* Take away the reference, and possibly free it */
 	sctp_free_ifn(sctp_ifnp);
 }
@@ -486,8 +491,8 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
     int dynamic_add)
 {
 	struct sctp_vrf *vrf;
-	struct sctp_ifn *sctp_ifnp = NULL;
-	struct sctp_ifa *sctp_ifap = NULL;
+	struct sctp_ifn *sctp_ifnp, *new_sctp_ifnp;
+	struct sctp_ifa *sctp_ifap, *new_sctp_ifap;
 	struct sctp_ifalist *hash_addr_head;
 	struct sctp_ifnlist *hash_ifn_head;
 	uint32_t hash_of_addr;
@@ -497,6 +502,23 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 	SCTPDBG(SCTP_DEBUG_PCB4, "vrf_id 0x%x: adding address: ", vrf_id);
 	SCTPDBG_ADDR(SCTP_DEBUG_PCB4, addr);
 #endif
+	SCTP_MALLOC(new_sctp_ifnp, struct sctp_ifn *,
+	    sizeof(struct sctp_ifn), SCTP_M_IFN);
+	if (new_sctp_ifnp == NULL) {
+#ifdef INVARIANTS
+		panic("No memory for IFN");
+#endif
+		return (NULL);
+	}
+	SCTP_MALLOC(new_sctp_ifap, struct sctp_ifa *, sizeof(struct sctp_ifa), SCTP_M_IFA);
+	if (new_sctp_ifap == NULL) {
+#ifdef INVARIANTS
+		panic("No memory for IFA");
+#endif
+		SCTP_FREE(new_sctp_ifnp, SCTP_M_IFN);
+		return (NULL);
+	}
+
 	SCTP_IPI_ADDR_WLOCK();
 	sctp_ifnp = sctp_find_ifn(ifn, ifn_index);
 	if (sctp_ifnp) {
@@ -507,6 +529,8 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 			vrf = sctp_allocate_vrf(vrf_id);
 			if (vrf == NULL) {
 				SCTP_IPI_ADDR_WUNLOCK();
+				SCTP_FREE(new_sctp_ifnp, SCTP_M_IFN);
+				SCTP_FREE(new_sctp_ifap, SCTP_M_IFA);
 				return (NULL);
 			}
 		}
@@ -516,15 +540,8 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 		 * build one and add it, can't hold lock until after malloc
 		 * done though.
 		 */
-		SCTP_IPI_ADDR_WUNLOCK();
-		SCTP_MALLOC(sctp_ifnp, struct sctp_ifn *,
-		    sizeof(struct sctp_ifn), SCTP_M_IFN);
-		if (sctp_ifnp == NULL) {
-#ifdef INVARIANTS
-			panic("No memory for IFN");
-#endif
-			return (NULL);
-		}
+		sctp_ifnp = new_sctp_ifnp;
+		new_sctp_ifnp = NULL;
 		memset(sctp_ifnp, 0, sizeof(struct sctp_ifn));
 		sctp_ifnp->ifn_index = ifn_index;
 		sctp_ifnp->ifn_p = ifn;
@@ -540,7 +557,6 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 		}
 		hash_ifn_head = &SCTP_BASE_INFO(vrf_ifn_hash)[(ifn_index & SCTP_BASE_INFO(vrf_ifn_hashmark))];
 		LIST_INIT(&sctp_ifnp->ifalist);
-		SCTP_IPI_ADDR_WLOCK();
 		LIST_INSERT_HEAD(hash_ifn_head, sctp_ifnp, next_bucket);
 		LIST_INSERT_HEAD(&vrf->ifnlist, sctp_ifnp, next_ifn);
 		atomic_add_int(&SCTP_BASE_INFO(ipi_count_ifns), 1);
@@ -567,6 +583,10 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 			}
 	exit_stage_left:
 			SCTP_IPI_ADDR_WUNLOCK();
+			if (new_sctp_ifnp != NULL) {
+				SCTP_FREE(new_sctp_ifnp, SCTP_M_IFN);
+			}
+			SCTP_FREE(new_sctp_ifap, SCTP_M_IFA);
 			return (sctp_ifap);
 		} else {
 			if (sctp_ifap->ifn_p) {
@@ -593,14 +613,7 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 			goto exit_stage_left;
 		}
 	}
-	SCTP_IPI_ADDR_WUNLOCK();
-	SCTP_MALLOC(sctp_ifap, struct sctp_ifa *, sizeof(struct sctp_ifa), SCTP_M_IFA);
-	if (sctp_ifap == NULL) {
-#ifdef INVARIANTS
-		panic("No memory for IFA");
-#endif
-		return (NULL);
-	}
+	sctp_ifap = new_sctp_ifap;
 	memset(sctp_ifap, 0, sizeof(struct sctp_ifa));
 	sctp_ifap->ifn_p = sctp_ifnp;
 	atomic_add_int(&sctp_ifnp->refcount, 1);
@@ -660,7 +673,6 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 	    (sctp_ifap->src_is_loop == 0)) {
 		sctp_ifap->src_is_glob = 1;
 	}
-	SCTP_IPI_ADDR_WLOCK();
 	hash_addr_head = &vrf->vrf_addr_hash[(hash_of_addr & vrf->vrf_addr_hashmark)];
 	LIST_INSERT_HEAD(hash_addr_head, sctp_ifap, next_bucket);
 	sctp_ifap->refcount = 1;
@@ -672,6 +684,10 @@ sctp_add_addr_to_vrf(uint32_t vrf_id, void *ifn, uint3
 		sctp_ifnp->registered_af = new_ifn_af;
 	}
 	SCTP_IPI_ADDR_WUNLOCK();
+	if (new_sctp_ifnp != NULL) {
+		SCTP_FREE(new_sctp_ifnp, SCTP_M_IFN);
+	}
+
 	if (dynamic_add) {
 		/*
 		 * Bump up the refcount so that when the timer completes it
@@ -5921,6 +5937,7 @@ retry:
 	 * free the vrf/ifn/ifa lists and hashes (be sure address monitor is
 	 * destroyed first).
 	 */
+	SCTP_IPI_ADDR_WLOCK();
 	vrf_bucket = &SCTP_BASE_INFO(sctp_vrfhash)[(SCTP_DEFAULT_VRFID & SCTP_BASE_INFO(hashvrfmark))];
 	LIST_FOREACH_SAFE(vrf, vrf_bucket, next_vrf, nvrf) {
 		LIST_FOREACH_SAFE(ifn, &vrf->ifnlist, next_ifn, nifn) {
@@ -5940,6 +5957,7 @@ retry:
 		LIST_REMOVE(vrf, next_vrf);
 		SCTP_FREE(vrf, SCTP_M_VRF);
 	}
+	SCTP_IPI_ADDR_WUNLOCK();
 	/* free the vrf hashes */
 	SCTP_HASH_FREE(SCTP_BASE_INFO(sctp_vrfhash), SCTP_BASE_INFO(hashvrfmark));
 	SCTP_HASH_FREE(SCTP_BASE_INFO(vrf_ifn_hash), SCTP_BASE_INFO(vrf_ifn_hashmark));

Modified: stable/12/sys/netinet/sctp_usrreq.c
==============================================================================
--- stable/12/sys/netinet/sctp_usrreq.c	Mon Aug 24 09:06:46 2020	(r364638)
+++ stable/12/sys/netinet/sctp_usrreq.c	Mon Aug 24 09:10:19 2020	(r364639)
@@ -984,9 +984,6 @@ sctp_fill_user_address(struct sockaddr *dst, struct so
 
 
 
-/*
- * NOTE: assumes addr lock is held
- */
 static size_t
 sctp_fill_up_addresses_vrf(struct sctp_inpcb *inp,
     struct sctp_tcb *stcb,
@@ -1006,6 +1003,7 @@ sctp_fill_up_addresses_vrf(struct sctp_inpcb *inp,
 #endif
 	struct sctp_vrf *vrf;
 
+	SCTP_IPI_ADDR_LOCK_ASSERT();
 	actual = 0;
 	if (limit == 0)
 		return (actual);
@@ -1238,9 +1236,6 @@ sctp_fill_up_addresses(struct sctp_inpcb *inp,
 	return (size);
 }
 
-/*
- * NOTE: assumes addr lock is held
- */
 static int
 sctp_count_max_addresses_vrf(struct sctp_inpcb *inp, uint32_t vrf_id)
 {
@@ -1254,6 +1249,7 @@ sctp_count_max_addresses_vrf(struct sctp_inpcb *inp, u
 	 * bound-all case a TCB may NOT include the loopback or other
 	 * addresses as well.
 	 */
+	SCTP_IPI_ADDR_LOCK_ASSERT();
 	vrf = sctp_find_vrf(vrf_id);
 	if (vrf == NULL) {
 		return (0);

Modified: stable/12/sys/netinet/sctputil.c
==============================================================================
--- stable/12/sys/netinet/sctputil.c	Mon Aug 24 09:06:46 2020	(r364638)
+++ stable/12/sys/netinet/sctputil.c	Mon Aug 24 09:10:19 2020	(r364639)
@@ -5287,8 +5287,11 @@ sctp_find_ifa_by_addr(struct sockaddr *addr, uint32_t 
 	struct sctp_ifalist *hash_head;
 	uint32_t hash_of_addr;
 
-	if (holds_lock == 0)
+	if (holds_lock == 0) {
 		SCTP_IPI_ADDR_RLOCK();
+	} else {
+		SCTP_IPI_ADDR_LOCK_ASSERT();
+	}
 
 	vrf = sctp_find_vrf(vrf_id);
 	if (vrf == NULL) {
@@ -6417,7 +6420,7 @@ sctp_dynamic_set_primary(struct sockaddr *sa, uint32_t
 	struct sctp_ifa *ifa;
 	struct sctp_laddr *wi;
 
-	ifa = sctp_find_ifa_by_addr(sa, vrf_id, 0);
+	ifa = sctp_find_ifa_by_addr(sa, vrf_id, SCTP_ADDR_NOT_LOCKED);
 	if (ifa == NULL) {
 		SCTP_LTRACE_ERR_RET(NULL, NULL, NULL, SCTP_FROM_SCTPUTIL, EADDRNOTAVAIL);
 		return (EADDRNOTAVAIL);



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