Date: Fri, 17 Jul 2020 15:09:49 +0000 (UTC) From: Michael Tuexen <tuexen@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r363275 - head/sys/netinet Message-ID: <202007171509.06HF9njS069308@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: tuexen Date: Fri Jul 17 15:09:49 2020 New Revision: 363275 URL: https://svnweb.freebsd.org/changeset/base/363275 Log: 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. MFC after: 1 week Modified: head/sys/netinet/sctp_lock_bsd.h head/sys/netinet/sctp_pcb.c head/sys/netinet/sctp_usrreq.c head/sys/netinet/sctputil.c Modified: head/sys/netinet/sctp_lock_bsd.h ============================================================================== --- head/sys/netinet/sctp_lock_bsd.h Fri Jul 17 14:51:51 2020 (r363274) +++ head/sys/netinet/sctp_lock_bsd.h Fri Jul 17 15:09:49 2020 (r363275) @@ -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: head/sys/netinet/sctp_pcb.c ============================================================================== --- head/sys/netinet/sctp_pcb.c Fri Jul 17 14:51:51 2020 (r363274) +++ head/sys/netinet/sctp_pcb.c Fri Jul 17 15:09:49 2020 (r363275) @@ -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 @@ -5906,6 +5922,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) { @@ -5925,6 +5942,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: head/sys/netinet/sctp_usrreq.c ============================================================================== --- head/sys/netinet/sctp_usrreq.c Fri Jul 17 14:51:51 2020 (r363274) +++ head/sys/netinet/sctp_usrreq.c Fri Jul 17 15:09:49 2020 (r363275) @@ -1004,9 +1004,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, @@ -1026,6 +1023,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); @@ -1258,9 +1256,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) { @@ -1274,6 +1269,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: head/sys/netinet/sctputil.c ============================================================================== --- head/sys/netinet/sctputil.c Fri Jul 17 14:51:51 2020 (r363274) +++ head/sys/netinet/sctputil.c Fri Jul 17 15:09:49 2020 (r363275) @@ -5296,8 +5296,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) { @@ -6429,7 +6432,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?202007171509.06HF9njS069308>