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>