From 6f456afe39a91e91c61e3218e4ea9faa19c1a4dd Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 26 Aug 2022 15:33:22 +0200 Subject: [PATCH 1/2] ike-sa-manager: Prevent new IKE_SA from getting created when flush() is called Without ability to create SPIs, other threads are prevented from creating new IKE_SAs while we are flushing existing IKE_SAs. However, there could still be IKE_SAs already created that might get checked in while the segments are temporarily unlocked to wait for threads to check existing SAs in. --- src/libcharon/sa/ike_sa_manager.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c index e860784de9..4ce567c21d 100644 --- a/src/libcharon/sa/ike_sa_manager.c +++ b/src/libcharon/sa/ike_sa_manager.c @@ -2391,6 +2391,14 @@ METHOD(ike_sa_manager_t, flush, void, entry_t *entry; u_int segment; + /* prevent threads from creating new SAs */ + this->spi_lock->write_lock(this->spi_lock); + DESTROY_IF(this->rng); + this->rng = NULL; + this->spi_cb.cb = NULL; + this->spi_cb.data = NULL; + this->spi_lock->unlock(this->spi_lock); + lock_all_segments(this); DBG2(DBG_MGR, "going to destroy IKE_SA manager and all managed IKE_SA's"); /* Step 1: drive out all waiting threads */ @@ -2431,13 +2439,6 @@ METHOD(ike_sa_manager_t, flush, void, /* Step 4: destroy all entries */ destroy_all_entries(this); unlock_all_segments(this); - - this->spi_lock->write_lock(this->spi_lock); - DESTROY_IF(this->rng); - this->rng = NULL; - this->spi_cb.cb = NULL; - this->spi_cb.data = NULL; - this->spi_lock->unlock(this->spi_lock); } METHOD(ike_sa_manager_t, destroy, void, From 2740c50bb8056fde570146fcd5e2c703bd54a840 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 26 Aug 2022 16:14:30 +0200 Subject: [PATCH 2/2] ike-sa-manager: Make sure flush() removes entries that might get added concurrently Because flush() has to release the segment locks intermittently, threads might add new entries (even with the change in the previous commit as the IKE_SA might already be created, just not registered/checked in yet). Since those entries are added to the front of the segment lists, the enumerator in the previous step 2 didn't notice them and did not wait for them to get checked in. However, step 3 and 4 then proceeded to delete and destroy the entry and IKE_SA, which could lead to a crash once the other thread attempts to check in the already destroyed IKE_SA. This change combines the three loops of steps 2-4 but then loops over the whole table until it's actually empty. This way we wait for and destroy newly added entries. --- src/libcharon/sa/ike_sa_manager.c | 132 ++++++++++++++++-------------- 1 file changed, 69 insertions(+), 63 deletions(-) diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c index 4ce567c21d..c8d4396302 100644 --- a/src/libcharon/sa/ike_sa_manager.c +++ b/src/libcharon/sa/ike_sa_manager.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2021 Tobias Brunner + * Copyright (C) 2008-2022 Tobias Brunner * Copyright (C) 2005-2011 Martin Willi * Copyright (C) 2005 Jan Hutter * @@ -65,14 +65,9 @@ struct entry_t { thread_t *checked_out; /** - * Does this SA drives out new threads? + * Whether threads are prevented from getting this IKE_SA. */ - bool driveout_new_threads; - - /** - * Does this SA drives out waiting threads? - */ - bool driveout_waiting_threads; + bool driveout_threads; /** * Identification of an IKE_SA (SPIs). @@ -350,6 +345,12 @@ struct private_ike_sa_manager_t { */ u_int segment_mask; + /** + * Enabled while shutting down to mark all new entries so no threads + * will check them out. + */ + bool new_entries_driveout_threads; + /** * Hash table with half_open_t objects. */ @@ -631,6 +632,8 @@ static u_int put_entry(private_ike_sa_manager_t *this, entry_t *entry) segment = row & this->segment_mask; lock_single_segment(this, segment); + /* mark entry during shutdown */ + entry->driveout_threads = this->new_entries_driveout_threads; current = this->ike_sa_table[row]; if (current) { /* insert at the front of current bucket */ @@ -759,12 +762,12 @@ static status_t get_entry_by_sa(private_ike_sa_manager_t *this, static bool wait_for_entry(private_ike_sa_manager_t *this, entry_t *entry, u_int segment) { - if (entry->driveout_new_threads) + if (entry->driveout_threads) { /* we are not allowed to get this */ return FALSE; } - while (entry->checked_out && !entry->driveout_waiting_threads) + while (entry->checked_out && !entry->driveout_threads) { /* so wait until we can get it for us. * we register us as waiting. */ @@ -773,7 +776,7 @@ static bool wait_for_entry(private_ike_sa_manager_t *this, entry_t *entry, entry->waiting_threads--; } /* hm, a deletion request forbids us to get this SA, get next one */ - if (entry->driveout_waiting_threads) + if (entry->driveout_threads) { /* we must signal here, others may be waiting on it, too */ entry->condvar->signal(entry->condvar); @@ -1761,7 +1764,7 @@ METHOD(ike_sa_manager_t, new_initiator_spi, bool, if (get_entry_by_sa(this, ike_sa_id, ike_sa, &entry, &segment) == SUCCESS) { - if (entry->driveout_waiting_threads && entry->driveout_new_threads) + if (entry->driveout_threads) { /* it looks like flush() has been called and the SA is being deleted * anyway, no need for a new SPI */ DBG2(DBG_MGR, "ignored change of initiator SPI during shutdown"); @@ -1833,8 +1836,7 @@ CALLBACK(enumerator_filter_skip, bool, while (orig->enumerate(orig, &entry, &segment)) { - if (!entry->driveout_new_threads && - !entry->driveout_waiting_threads && + if (!entry->driveout_threads && !entry->checked_out) { *out = entry->ike_sa; @@ -1997,7 +1999,7 @@ METHOD(ike_sa_manager_t, checkin_and_destroy, void, if (get_entry_by_sa(this, ike_sa_id, ike_sa, &entry, &segment) == SUCCESS) { - if (entry->driveout_waiting_threads && entry->driveout_new_threads) + if (entry->driveout_threads) { /* it looks like flush() has been called and the SA is being deleted * anyway, just check it in */ DBG2(DBG_MGR, "ignored checkin and destroy of IKE_SA during shutdown"); @@ -2007,10 +2009,8 @@ METHOD(ike_sa_manager_t, checkin_and_destroy, void, return; } - /* drive out waiting threads, as we are in hurry */ - entry->driveout_waiting_threads = TRUE; - /* mark it, so no new threads can get this entry */ - entry->driveout_new_threads = TRUE; + /* mark it, so no threads can get this entry */ + entry->driveout_threads = TRUE; /* wait until all workers have done their work */ while (entry->waiting_threads) { @@ -2351,6 +2351,29 @@ METHOD(ike_sa_manager_t, set_spi_cb, void, this->spi_lock->unlock(this->spi_lock); } +/** + * Destroy a single entry + */ +static void remove_and_destroy_entry(private_ike_sa_manager_t *this, + enumerator_t *enumerator, entry_t *entry) +{ + if (entry->half_open) + { + remove_half_open(this, entry->other, + entry->ike_sa_id->is_initiator(entry->ike_sa_id)); + } + if (entry->my_id && entry->other_id) + { + remove_connected_peers(this, entry); + } + if (entry->init_hash.ptr) + { + remove_init_hash(this, entry->init_hash); + } + remove_entry_at((private_enumerator_t*)enumerator); + entry_destroy(entry); +} + /** * Destroy all entries */ @@ -2364,21 +2387,7 @@ static void destroy_all_entries(private_ike_sa_manager_t *this) while (enumerator->enumerate(enumerator, &entry, &segment)) { charon->bus->set_sa(charon->bus, entry->ike_sa); - if (entry->half_open) - { - remove_half_open(this, entry->other, - entry->ike_sa_id->is_initiator(entry->ike_sa_id)); - } - if (entry->my_id && entry->other_id) - { - remove_connected_peers(this, entry); - } - if (entry->init_hash.ptr) - { - remove_init_hash(this, entry->init_hash); - } - remove_entry_at((private_enumerator_t*)enumerator); - entry_destroy(entry); + remove_and_destroy_entry(this, enumerator, entry); } enumerator->destroy(enumerator); charon->bus->set_sa(charon->bus, NULL); @@ -2400,44 +2409,41 @@ METHOD(ike_sa_manager_t, flush, void, this->spi_lock->unlock(this->spi_lock); lock_all_segments(this); - DBG2(DBG_MGR, "going to destroy IKE_SA manager and all managed IKE_SA's"); - /* Step 1: drive out all waiting threads */ - DBG2(DBG_MGR, "set driveout flags for all stored IKE_SA's"); + DBG2(DBG_MGR, "going to destroy IKE_SA manager and all managed IKE_SAs"); enumerator = create_table_enumerator(this); while (enumerator->enumerate(enumerator, &entry, &segment)) { - /* do not accept new threads, drive out waiting threads */ - entry->driveout_new_threads = TRUE; - entry->driveout_waiting_threads = TRUE; + /* mark all entries so threads can't check these IKE_SAs out */ + entry->driveout_threads = TRUE; } enumerator->destroy(enumerator); - DBG2(DBG_MGR, "wait for all threads to leave IKE_SA's"); - /* Step 2: wait until all are gone */ - enumerator = create_table_enumerator(this); - while (enumerator->enumerate(enumerator, &entry, &segment)) + DBG2(DBG_MGR, "wait for threads to leave IKE_SAs and delete and destroy " + "them"); + /* we are intermittently unlocking the segments below, which allows threads + * to create new entries. we want these to get marked directly so other + * threads can't get them again. we re-enumerate the table until all + * entries are gone */ + this->new_entries_driveout_threads = TRUE; + while (ref_cur(&this->total_sa_count)) { - while (entry->waiting_threads || entry->checked_out) + enumerator = create_table_enumerator(this); + while (enumerator->enumerate(enumerator, &entry, &segment)) { - /* wake up all */ - entry->condvar->broadcast(entry->condvar); - /* go sleeping until they are gone */ - entry->condvar->wait(entry->condvar, this->segments[segment].mutex); + while (entry->waiting_threads || entry->checked_out) + { + /* wake up all */ + entry->condvar->broadcast(entry->condvar); + /* go sleeping until they are gone */ + entry->condvar->wait(entry->condvar, + this->segments[segment].mutex); + } + charon->bus->set_sa(charon->bus, entry->ike_sa); + entry->ike_sa->delete(entry->ike_sa, TRUE); + remove_and_destroy_entry(this, enumerator, entry); + charon->bus->set_sa(charon->bus, NULL); } + enumerator->destroy(enumerator); } - enumerator->destroy(enumerator); - DBG2(DBG_MGR, "delete all IKE_SA's"); - /* Step 3: initiate deletion of all IKE_SAs */ - enumerator = create_table_enumerator(this); - while (enumerator->enumerate(enumerator, &entry, &segment)) - { - charon->bus->set_sa(charon->bus, entry->ike_sa); - entry->ike_sa->delete(entry->ike_sa, TRUE); - } - enumerator->destroy(enumerator); - - DBG2(DBG_MGR, "destroy all entries"); - /* Step 4: destroy all entries */ - destroy_all_entries(this); unlock_all_segments(this); }